Neos 9 compatibility (event-sourced Content Repository) — new 4.x line - #28
Conversation
|
Warning Review limit reached
Next review available in: 38 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR migrates the extension to Neos 9 Content Repository APIs. It updates graph reads, event-sourced patch commands, workspace handling, Fusion integration, functional tests, dependencies, CI, and compatibility documentation. ChangesNeos 9 repository integration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (14)
Classes/Service/PatchValidator.php (2)
375-388: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidation and execution build the
TransientNodedifferently for updates.Here updates use
NodeAggregateIdsByNodePaths::createForNodeType(...), whileNodePatchService::executeUpdateNode()usescreateEmpty(). Divergent transient-node setup can make a patch pass pre-validation and then fail during execution (partial application, no rollback). Worth aligning both paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Classes/Service/PatchValidator.php` around lines 375 - 388, Align the update validation setup in the code surrounding TransientNode::forRegular with NodePatchService::executeUpdateNode(): use NodeAggregateIdsByNodePaths::createEmpty() instead of createForNodeType(...). Keep the existing setup for non-update validation paths unchanged.
223-240: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueUnknown node types silently skip the move constraint check.
Unlike
validateCreateNodePatch/validateUpdateNodePatch, which throw when a type is not in the schema, this condition just bypasses validation when either type resolves tonull. Prefer an explicit failure for consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Classes/Service/PatchValidator.php` around lines 223 - 240, Update the node type validation in the move validation flow to explicitly throw a PatchFailedException when either $newParentNodeType or $nodeType is null, matching the unknown-type behavior of validateCreateNodePatch and validateUpdateNodePatch. Only perform allowsChildNodeType validation after both node types resolve successfully, preserving the existing rejection for disallowed child types.Classes/Service/NodePatchService.php (2)
740-766: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNode-identifier parsing duplicated across the patch services. Both services independently parse
nodeIdas either a plainNodeAggregateIdor aNodeAddressJSON string and resolve it in the subgraph; the accepted formats can drift between validation and execution.
Classes/Service/NodePatchService.php#L740-L766: replace the inline parsing inrequireNode()with a call to a shared resolver helper.Classes/Service/PatchValidator.php#L297-L322: replace the identical parsing ingetNodeById()with the same shared resolver.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Classes/Service/NodePatchService.php` around lines 740 - 766, Extract the duplicated nodeId parsing and subgraph lookup into a shared resolver helper, then update NodePatchService::requireNode() in Classes/Service/NodePatchService.php lines 740-766 to use it while preserving its existing PatchFailedException handling. Update PatchValidator::getNodeById() in Classes/Service/PatchValidator.php lines 297-322 to use the same helper, ensuring both services accept identical plain NodeAggregateId and NodeAddress JSON formats.
268-277: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse
hasNodeType()beforegetNodeType(), asPatchValidatordoes.
PatchValidator::getNodeType()documents thatgetNodeType()returns a fallback node type rather thannullwhen a fallback is configured, so this null-check can pass for a non-existent type. Same guard should apply here.♻️ Proposed change
$nodeTypeManager = $contentRepository->getNodeTypeManager(); - $nodeType = $nodeTypeManager->getNodeType($patch->getNodeType()); - if ($nodeType === null) { + $nodeType = $nodeTypeManager->hasNodeType($patch->getNodeType()) + ? $nodeTypeManager->getNodeType($patch->getNodeType()) + : null; + if ($nodeType === null) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Classes/Service/NodePatchService.php` around lines 268 - 277, Update the node type validation in the patch handling method containing $nodeTypeManager to call hasNodeType($patch->getNodeType()) before getNodeType(). Throw the existing PatchFailedException when the type is absent, then retrieve it with getNodeType() only after validation; do not rely on a null check because fallback configuration can return a node type for unknown names.Tests/E2E/tests/neos9.editorsAndChat.spec.ts (1)
19-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueType the
pageparameter.Implicit
anyhere loses autocompletion and breaks undernoImplicitAnyif the suite is ever type-checked.♻️ Proposed tweak
-import { test, expect } from '`@playwright/test`'; +import { test, expect, type Page } from '`@playwright/test`'; @@ -const dockSidebar = async (page) => { +const dockSidebar = async (page: Page) => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/E2E/tests/neos9.editorsAndChat.spec.ts` around lines 19 - 23, Type the page parameter in dockSidebar using the appropriate Playwright Page type already used by the test suite, preserving the existing initialization behavior.Tests/Functional/FunctionalTestCase.php (1)
360-397: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the created document is resolvable before returning.
findNodeById()is nullable while the method is declared: Node; a projection miss would surface as aTypeErrorinstead of a readable failure, unlike the tethered-child check on Line 381.♻️ Proposed tweak
- return $subgraph->findNodeById($documentNodeAggregateId); + $documentNode = $subgraph->findNodeById($documentNodeAggregateId); + $this->assertNotNull($documentNode, sprintf('Document node "%s" was not created', $nodeName)); + + return $documentNode;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/Functional/FunctionalTestCase.php` around lines 360 - 397, Update createPageWithImageNodes to assert that findNodeById($documentNodeAggregateId) returns a non-null Node before returning it, matching the existing assertion for the tethered main collection and producing a readable failure on projection misses.composer.json (1)
18-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid
dev-masteras a production fallback.
^9.0 || dev-masterallows future Neos development code to satisfy the dependency and can silently introduce incompatible APIs. Composer explicitly warns that unbounded constraints such asdev-masterallow updates to future versions. Keep the stable constraint here, or isolate the development constraint in an explicit compatibility-testing profile. (getcomposer.org)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@composer.json` around lines 18 - 20, Remove the dev-master alternatives from the neos/neos, neos/media, and neos/neos-ui dependency constraints in composer.json, leaving only the stable ^9.0 requirements. Do not add an unbounded development fallback; isolate any development compatibility constraint separately if needed.Source: MCP tools
Classes/Service/NodeWithImageService.php (1)
99-104: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueLanguage filter silently passes documents without the language coordinate.
When
$languageCoordinate === nullthe document is kept even though a language filter was requested. If that is intentional (dimension-less installations), a short comment would help; otherwise it lets unfiltered documents through.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Classes/Service/NodeWithImageService.php` around lines 99 - 104, Update the language-filter logic in NodeWithImageService to exclude documents when a language filter is requested and getCoordinate returns null, while preserving documents whose coordinate matches the filter. If null is intentionally allowed for dimension-less installations, document that behavior with a concise comment instead.Classes/Service/NodeTreeExtractor.php (1)
42-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo overlapping content-repository dependencies.
extractChildren()resolves the CR from$node->contentRepositoryIdvia the registry whileextract()usescontentRepositoryProvider. Since the node always originates from the provider's repository, dropping the registry injection (or the provider) would keep one source of truth;contentRepositoryRegistry->subgraphForNode($node)can stay.Also applies to: 228-234
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Classes/Service/NodeTreeExtractor.php` around lines 42 - 45, Consolidate the content-repository dependency in NodeTreeExtractor by removing the redundant injection and updating extract() and extractChildren() to use one consistent repository source. Preserve the existing subgraphForNode($node) behavior while ensuring both methods resolve the repository associated with the provider’s node.Classes/Service/DocumentNodeListExtractor.php (1)
115-135: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused
$contentRepositoryparameter inresolveSiteNode().Flagged by PHPMD; the parameter is never read in the method body.
🧹 Suggested cleanup
- private function resolveSiteNode(ContentRepository $contentRepository, ContentSubgraphInterface $subgraph, ?string $siteNodeName): ?Node + private function resolveSiteNode(ContentSubgraphInterface $subgraph, ?string $siteNodeName): ?Node {And update the call site:
$this->resolveSiteNode($subgraph, $siteNodeName).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Classes/Service/DocumentNodeListExtractor.php` around lines 115 - 135, Remove the unused ContentRepository parameter from resolveSiteNode() and update its call site to pass only $subgraph and $siteNodeName; preserve the existing node-resolution behavior.Source: Linters/SAST tools
Classes/Controller/PreviewRenderController.php (1)
167-207: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLegacy-dimensions-to-
DimensionSpacePointmapping duplicated across two files. Both implementations independently convert a legacy fallback-chain dimensions array into aDimensionSpacePoint, using the primary/first value per dimension and falling back to the variation graph's root generalization when no dimensions are given — the same root cause (no shared helper) asNodeVisibility.php, which centralizes a similar cross-cutting Neos 9 concern.
Classes/Controller/PreviewRenderController.php#L167-L207: extractdimensionCoordinatesFromLegacyDimensionsArray()+getDefaultDimensionSpacePoint()into a shared service (e.g. alongsideNodeVisibility) and delegate to it here.Classes/Service/DocumentNodeListExtractor.php#L208-L220: replaceresolveDimensionSpacePoint()'s body with a call to the same shared helper instead of reimplementing the mapping.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Classes/Controller/PreviewRenderController.php` around lines 167 - 207, Extract the legacy dimension-coordinate mapping and variation-graph default selection from PreviewRenderController methods dimensionCoordinatesFromLegacyDimensionsArray() and getDefaultDimensionSpacePoint() into a shared service alongside NodeVisibility, preserving primary fallback values and root-generalization behavior. Delegate PreviewRenderController to that service, and replace resolveDimensionSpacePoint() in Classes/Service/DocumentNodeListExtractor.php lines 208-220 with the same helper call; both sites should use the shared implementation rather than duplicate the logic.Classes/Service/NodeService.php (1)
203-241: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
updatePropertiesOnNode()propagates raw parse failures to the API caller.
NodeAddress::fromJsonString()on Line 227 is unguarded, so a malformednodeContextPathfrom the client surfaces as an unhandled exception rather than theInvalidArgumentExceptioncontract used two lines below for the missing-node case. Wrapping it keeps the failure mode uniform forApplyPatchesApi/BackendServiceconsumers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Classes/Service/NodeService.php` around lines 203 - 241, Update updatePropertiesOnNode() to catch parse failures from NodeAddress::fromJsonString() and rethrow them as InvalidArgumentException, preserving the original error as the cause and using the same client-facing contract as the missing-node branch. Leave valid address processing and the existing missing-node exception unchanged.Classes/Factory/FindImageDataFactory.php (1)
98-115: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
resolveNodeOrderIndex()adds two graph queries per image node.
createFromNodeAndSchema()is invoked once per matching content node inNodeWithImageService::findDocumentNodesHavingChildNodesWithImages(), which walks every descendant of every document. Each call does afindParentNode()plus a fullfindChildNodes()fetch of the sibling set, so a page with many image nodes re-fetches the same sibling list repeatedly. A per-request memo keyed by parent aggregate id + subgraph identity removes the repetition without changing semantics.♻️ Memoize sibling positions per parent
+ /** `@var` array<string, array<string, int>> */ + private array $orderIndexCache = []; + private function resolveNodeOrderIndex(Node $node): int { $subgraph = $this->contentRepositoryRegistry->subgraphForNode($node); $parentNode = $subgraph->findParentNode($node->aggregateId); if ($parentNode === null) { return 0; } - $position = 0; - foreach ($subgraph->findChildNodes($parentNode->aggregateId, FindChildNodesFilter::create()) as $siblingNode) { - if ($siblingNode->aggregateId->equals($node->aggregateId)) { - return $position; - } - $position++; - } - - return 0; + $cacheKey = $node->workspaceName->value . '|' . $node->dimensionSpacePoint->hash . '|' . $parentNode->aggregateId->value; + if (!isset($this->orderIndexCache[$cacheKey])) { + $positions = []; + $position = 0; + foreach ($subgraph->findChildNodes($parentNode->aggregateId, FindChildNodesFilter::create()) as $siblingNode) { + $positions[$siblingNode->aggregateId->value] = $position++; + } + $this->orderIndexCache[$cacheKey] = $positions; + } + + return $this->orderIndexCache[$cacheKey][$node->aggregateId->value] ?? 0; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Classes/Factory/FindImageDataFactory.php` around lines 98 - 115, Memoize sibling positions in resolveNodeOrderIndex() to avoid repeating findParentNode() and findChildNodes() queries for image nodes sharing the same parent and subgraph. Add a per-request cache keyed by the parent aggregate ID and subgraph identity, populate it once from the sibling list, and reuse the cached node positions while preserving the existing zero fallback semantics.Classes/Service/AssetService.php (1)
74-91: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDropping
AssetRepository::iterate()also drops its periodic identity-map cleanup.Flow's
iterate()detached/cleared hydrated entities every N rows; plaintoIterable()keeps every hydratedImagein the Doctrine identity map for the whole loop. Combined with the offset emulation ($iteratedItems <= getFirstResult()), a deep page over a large media library now hydrates and retains all skipped rows. Consider detaching skipped assets, or moving offset/limit into the query itself.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Classes/Service/AssetService.php` around lines 74 - 91, Update the asset iteration in the service method using $assetsIterator so deep pagination does not retain every skipped entity in Doctrine’s identity map. Prefer applying the first-result offset and limit to the query before iteration; otherwise detach skipped $currentAsset entities while preserving the existing usage filtering and result-count behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test.yml:
- Line 50: Update the setup-php configuration’s extensions list to replace the
removed mysql extension with pdo_mysql, matching the workflow’s pdo_mysql driver
requirement, and add fail-fast: true under that setup step so installation
failures stop the job immediately.
In `@Classes/Controller/PreviewRenderController.php`:
- Around line 150-165: The findNode() method currently fetches its subgraph
through contentRepository->getContentSubgraph(), allowing authorization-derived
visibility. Replace that fetch with the package’s getSubgraph() path and
explicitly pass NodeVisibility::excludeRemoved(), while preserving the existing
workspace, dimension-space point, node lookup, and exception handling.
In `@Classes/EelHelper/NEOSidekickInternalHelper.php`:
- Around line 100-104: Update contentDimensionsByName() to stop instantiating
the legacy Neos\Neos\Ui\Fusion\Helper\ContentDimensionsHelper and retrieve the
dimensions through the supported Neos\Neos\Fusion\Helper\DimensionHelper or
equivalent Neos.Dimension Eel helper API, preserving the existing content
repository context and array result.
In `@Classes/Service/NodePatchService.php`:
- Around line 357-375: Reorder the hidden-node creation flow so
`$subgraph->findNodeById($newNodeAggregateId)` and its null check occur before
dispatching `DisableNodeAggregate`. Keep the existing disable operation
afterward, ensuring hidden nodes are validated while visible and avoiding
dependence on subgraph visibility constraints.
- Around line 160-174: Update the Apply Patches response documentation in
ApplyPatchesApiController.php and README_Internal_API.md to reflect dry-run
results containing only index, operation, and nodeId. Remove createdNodes and
any previewed node-detail examples from the dry-run response documentation while
preserving the documented non-dry-run response behavior.
In `@Classes/Service/NodeService.php`:
- Around line 312-326: Update nodeMatchesLanguageDimensionFilter() to return
true when getLanguageDimensionFilter() is empty, treating an unset language
filter as no restriction. Preserve the existing dimension-source and coordinate
checks for non-empty filters so find() and findImportantPages() behave
consistently.
- Around line 82-104: In findImportantPages, normalize the nullable result of
FindDocumentNodesFilter::getLanguageDimensionFilter() to an empty array once
before the language-dimension loop, then use that normalized variable for the
empty check and membership filtering. Apply the same normalization in the other
in_array usage identified in the diff, preserving existing behavior for non-null
filters.
In `@Classes/Service/NodeTreeExtractor.php`:
- Around line 59-64: Update the subgraph lookup in NodeTreeExtractor to resolve
an empty dimensions array through the existing variation-graph
root-generalization fallback used by
SearchNodesExtractor::resolveDimensionSpacePoint(), rather than passing an empty
DimensionSpacePoint to getContentSubgraph. Apply the same change to the
additional lookup path around the referenced lines while preserving explicit
dimension coordinates.
In `@Classes/Service/NodeWithImageService.php`:
- Around line 75-95: Update NodeWithImageService to reference the global
InvalidArgumentException explicitly (or import it) for both the
missing-workspace throw and the NodeAddress::fromJsonString() catch. Verify
fromJsonString() throws PHP’s \InvalidArgumentException in Neos 9; if it uses a
different exception type, catch that exact type instead so malformed addresses
are skipped and logged.
In `@Classes/Service/SearchNodesExtractor.php`:
- Around line 167-172: Update resolveNodeByIdentifier() to normalize
$effectiveNodeTypeFilter using the same NodeTypeCriteria parsing as
findDescendantNodes() before calling NodeType::isOfType(). Preserve support for
comma-separated and !* filters, and use the normalized criteria when evaluating
the node-type condition.
In `@README.md`:
- Around line 18-21: Update the README installation example to require the Neos
9-compatible plugin major, replacing the neosidekick/ai-assistant ^2.5
constraint with ^3.0 or an equivalent version-neutral constraint consistent with
the documented release availability.
In `@Resources/Private/BackendModule/BackendModule.fusion`:
- Around line 74-81: Update the workspaces value in the Neos.Fusion:Value block
to use the workspace value object that provides the configured workspace data,
rather than relying on
Neos.Ui.Workspace.getPersonalWorkspace(contentRepositoryId). Preserve the
allowedTargetWorkspaces mapping and JSON serialization so props.workspaces
contains the personal and allowed workspaces for switcher initialization.
In `@TESTING.md`:
- Around line 65-73: Update TESTING.md lines 65-73 to remove the unconditional
“ANY Neos 9 site” and “works out of the box” claims, and state that eligibility
follows each spec’s requirements, including a publicly reachable base URL and an
image asset with an empty title for neos9.bulkAltTextModule.spec.ts. Update
Tests/E2E/README.md lines 34-39 to remove “without extra content” and make Neos
9 E2E eligibility conditional on the documented per-spec requirements.
In `@Tests/E2E/tests/neos9.bulkAltTextModule.spec.ts`:
- Around line 47-55: Replace the fixed page.waitForTimeout(5_000) after
saveAll.click() with an explicit wait for the save completion signal, such as
the updateAssets response, confirmation UI, next-page navigation, or saved-state
UI. Keep the existing no-alert assertion and ensure they run only after the save
has completed.
- Around line 36-39: Update the persistence check around firstImageSrc to
capture a stable identifier for the asset being edited instead of relying on the
first image source. After saving, explicitly wait for the post-save list state
using an assertion for the empty state or rendered list, then verify that
specific asset is absent or its persisted title is present; do not use immediate
isVisible checks or list ordering as the synchronization/identity mechanism.
In `@Tests/E2E/tests/neos9.editorsAndChat.spec.ts`:
- Around line 74-104: The chat-readiness check using chatInput().isVisible({
timeout: 30_000 }) is a one-shot check and should use a retry-based wait
instead. Update the chat input readiness flow to call waitFor({ state:
'visible', timeout: 30_000 }) or expect(...).toBeVisible(...) with the existing
catch-to-false behavior, while preserving the page.reload() fallback when the
wait fails.
In `@Tests/Functional/Service/NodeServiceImportantPagesMultiDomainTest.php`:
- Around line 35-42: The test expectation for
getMostRelevantInternalSeoUrisByHosts must match
NodeService::findImportantPages() when no language dimension exists. In the
relevant test method, either skip the dimension-dependent test when
primaryLanguage() is null or construct expected hosts without a trailing slash
in that case, while preserving the current language-segment expectations for
dimension-enabled projects.
In
`@Tests/Functional/Service/NodeServiceWithImportantPagesFilterAndMultipleDimensionsAndOneSiteTest.php`:
- Around line 69-72: Update the forbidden-node assertion in the test’s
$foundNodes verification to compare the result-array key correctly. Replace the
per-DTO getNodeContextPath() comparison with assertArrayNotHasKey($forbiddenKey,
$foundNodes), preserving the existing count assertion and test intent.
---
Nitpick comments:
In `@Classes/Controller/PreviewRenderController.php`:
- Around line 167-207: Extract the legacy dimension-coordinate mapping and
variation-graph default selection from PreviewRenderController methods
dimensionCoordinatesFromLegacyDimensionsArray() and
getDefaultDimensionSpacePoint() into a shared service alongside NodeVisibility,
preserving primary fallback values and root-generalization behavior. Delegate
PreviewRenderController to that service, and replace
resolveDimensionSpacePoint() in Classes/Service/DocumentNodeListExtractor.php
lines 208-220 with the same helper call; both sites should use the shared
implementation rather than duplicate the logic.
In `@Classes/Factory/FindImageDataFactory.php`:
- Around line 98-115: Memoize sibling positions in resolveNodeOrderIndex() to
avoid repeating findParentNode() and findChildNodes() queries for image nodes
sharing the same parent and subgraph. Add a per-request cache keyed by the
parent aggregate ID and subgraph identity, populate it once from the sibling
list, and reuse the cached node positions while preserving the existing zero
fallback semantics.
In `@Classes/Service/AssetService.php`:
- Around line 74-91: Update the asset iteration in the service method using
$assetsIterator so deep pagination does not retain every skipped entity in
Doctrine’s identity map. Prefer applying the first-result offset and limit to
the query before iteration; otherwise detach skipped $currentAsset entities
while preserving the existing usage filtering and result-count behavior.
In `@Classes/Service/DocumentNodeListExtractor.php`:
- Around line 115-135: Remove the unused ContentRepository parameter from
resolveSiteNode() and update its call site to pass only $subgraph and
$siteNodeName; preserve the existing node-resolution behavior.
In `@Classes/Service/NodePatchService.php`:
- Around line 740-766: Extract the duplicated nodeId parsing and subgraph lookup
into a shared resolver helper, then update NodePatchService::requireNode() in
Classes/Service/NodePatchService.php lines 740-766 to use it while preserving
its existing PatchFailedException handling. Update PatchValidator::getNodeById()
in Classes/Service/PatchValidator.php lines 297-322 to use the same helper,
ensuring both services accept identical plain NodeAggregateId and NodeAddress
JSON formats.
- Around line 268-277: Update the node type validation in the patch handling
method containing $nodeTypeManager to call hasNodeType($patch->getNodeType())
before getNodeType(). Throw the existing PatchFailedException when the type is
absent, then retrieve it with getNodeType() only after validation; do not rely
on a null check because fallback configuration can return a node type for
unknown names.
In `@Classes/Service/NodeService.php`:
- Around line 203-241: Update updatePropertiesOnNode() to catch parse failures
from NodeAddress::fromJsonString() and rethrow them as InvalidArgumentException,
preserving the original error as the cause and using the same client-facing
contract as the missing-node branch. Leave valid address processing and the
existing missing-node exception unchanged.
In `@Classes/Service/NodeTreeExtractor.php`:
- Around line 42-45: Consolidate the content-repository dependency in
NodeTreeExtractor by removing the redundant injection and updating extract() and
extractChildren() to use one consistent repository source. Preserve the existing
subgraphForNode($node) behavior while ensuring both methods resolve the
repository associated with the provider’s node.
In `@Classes/Service/NodeWithImageService.php`:
- Around line 99-104: Update the language-filter logic in NodeWithImageService
to exclude documents when a language filter is requested and getCoordinate
returns null, while preserving documents whose coordinate matches the filter. If
null is intentionally allowed for dimension-less installations, document that
behavior with a concise comment instead.
In `@Classes/Service/PatchValidator.php`:
- Around line 375-388: Align the update validation setup in the code surrounding
TransientNode::forRegular with NodePatchService::executeUpdateNode(): use
NodeAggregateIdsByNodePaths::createEmpty() instead of createForNodeType(...).
Keep the existing setup for non-update validation paths unchanged.
- Around line 223-240: Update the node type validation in the move validation
flow to explicitly throw a PatchFailedException when either $newParentNodeType
or $nodeType is null, matching the unknown-type behavior of
validateCreateNodePatch and validateUpdateNodePatch. Only perform
allowsChildNodeType validation after both node types resolve successfully,
preserving the existing rejection for disallowed child types.
In `@composer.json`:
- Around line 18-20: Remove the dev-master alternatives from the neos/neos,
neos/media, and neos/neos-ui dependency constraints in composer.json, leaving
only the stable ^9.0 requirements. Do not add an unbounded development fallback;
isolate any development compatibility constraint separately if needed.
In `@Tests/E2E/tests/neos9.editorsAndChat.spec.ts`:
- Around line 19-23: Type the page parameter in dockSidebar using the
appropriate Playwright Page type already used by the test suite, preserving the
existing initialization behavior.
In `@Tests/Functional/FunctionalTestCase.php`:
- Around line 360-397: Update createPageWithImageNodes to assert that
findNodeById($documentNodeAggregateId) returns a non-null Node before returning
it, matching the existing assertion for the tethered main collection and
producing a readable failure on projection misses.
🪄 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 Plus
Run ID: e54fc496-b474-4c47-aef6-a1ee93e5aae9
📒 Files selected for processing (43)
.github/workflows/test.ymlClasses/Controller/BackendModule/AbstractFusionViewController.phpClasses/Controller/BackendServiceController.phpClasses/Controller/PreviewRenderController.phpClasses/EelHelper/NEOSidekickHelper.phpClasses/EelHelper/NEOSidekickInternalHelper.phpClasses/Factory/FindDocumentNodeDataFactory.phpClasses/Factory/FindImageDataFactory.phpClasses/Service/AbstractNodeService.phpClasses/Service/AssetService.phpClasses/Service/ContentRepositoryProvider.phpClasses/Service/DocumentNodeListExtractor.phpClasses/Service/NodeFindingService.phpClasses/Service/NodePatchService.phpClasses/Service/NodeService.phpClasses/Service/NodeTreeExtractor.phpClasses/Service/NodeTypeSchemaExtractor.phpClasses/Service/NodeTypeService.phpClasses/Service/NodeVisibility.phpClasses/Service/NodeWithImageService.phpClasses/Service/PatchValidator.phpClasses/Service/PropertyNormalizer.phpClasses/Service/SearchNodesExtractor.phpClasses/Service/SiteService.phpClasses/Service/Traits/PropertyExtractionTrait.phpConfiguration/Settings.Internal.yamlConfiguration/Settings.yamlConfiguration/Testing/NodeTypes.HomePage.yamlConfiguration/Testing/Settings.yamlREADME.mdResources/Private/BackendModule/BackendModule.fusionResources/Private/BackendModule/Root.fusionTESTING.mdTests/E2E/README.mdTests/E2E/tests/neos9.bulkAltTextModule.spec.tsTests/E2E/tests/neos9.editorsAndChat.spec.tsTests/Functional/FunctionalTestCase.phpTests/Functional/Service/NodeServiceImportantPagesMultiDomainTest.phpTests/Functional/Service/NodeServiceMultiSiteTest.phpTests/Functional/Service/NodeServiceWithImportantPagesFilterAndMultipleDimensionsAndOneSiteTest.phpTests/Functional/Service/NodeServiceWithMultipleDimensionsAndOneSiteTest.phpTests/Functional/Service/NodeServiceWorkspacesTest.phpcomposer.json
💤 Files with no reviewable changes (3)
- Resources/Private/BackendModule/Root.fusion
- Classes/Service/SiteService.php
- Classes/Controller/BackendServiceController.php
| with: | ||
| php-version: ${{ matrix.php-versions }} | ||
| extensions: mbstring, xml, json, zlib, iconv, intl, pdo_sqlite, mysql, pgsql, redis, memcached, memcache, apcu | ||
| extensions: mbstring, xml, json, zlib, iconv, intl, mysql, pgsql, redis, memcached, memcache, apcu |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
set -eu
printf 'Tracked workflow files:\n'
git ls-files '.github/workflows/*'
printf '\nRelevant setup-php lines:\n'
if [ -f .github/workflows/test.yml ]; then
nl -ba .github/workflows/test.yml | sed -n '1,110p'
fi
printf '\nSearch for pdo/mysql references in workflow and composer/config:\n'
rg -n "pdo|mysql|driver:\s*pdo|extensions:|setup-php|fail-fast" -S .github composer.json src 2>/dev/null || trueRepository: NEOSidekick/NEOSidekick.AiAssistant
Length of output: 1064
🌐 Web query:
shivammathur/setup-php driver pdo_mysql extensions pdo_mysql fail-fast env
💡 Result:
To configure the pdo_mysql extension in the shivammathur/setup-php GitHub Action and control how the workflow responds to installation failures, you should use the extensions input and the fail-fast environment variable [1][2]. Configuration Example: - name: Setup PHP uses: shivammathur/setup-php@v2 with: php-version: '8.5' extensions: pdo_mysql env: fail-fast: true Extension Management: The extensions input accepts a comma-separated list of extensions [3][4]. By adding pdo_mysql to this list, the action will attempt to install and enable the driver [5][4]. Fail-Fast Behavior: By default, shivammathur/setup-php operates in a "graceful" mode, meaning if an extension (or tool) fails to install, the action will log a warning but allow the workflow to continue [2][6]. Setting the fail-fast environment variable to true (as shown in the example above) changes this behavior [1][2]. With fail-fast: true, the workflow will immediately mark the step as failed if any specified extension or tool cannot be successfully set up [1][2]. This is useful for ensuring that your CI/CD environment is strictly configured according to your project requirements [7].
Citations:
- 1: https://shivammathur-setup-php.mintlify.app/configuration/flags
- 2: https://github.com/shivammathur/setup-php?tab=readme-ov-file
- 3: https://mintlify.wiki/shivammathur/setup-php/configuration/inputs
- 4: https://github.com/shivammathur/setup-php
- 5: https://shivammathur-setup-php.mintlify.app/
- 6: https://github.com/shivammathur/setup-php/blob/master/README.md
- 7: Add a way to turn off graceful logging shivammathur/setup-php#306
🌐 Web query:
PHP removed ext mysql pgsql extension name PDO mysql setup-php
💡 Result:
The original ext/mysql extension (often referred to as the mysql_* functions) was officially deprecated in PHP 5.5.0 and completely removed in PHP 7.0.0 [1][2][3]. It was removed because the API had become outdated, lacked support for modern features, and presented significant maintenance and security challenges [3]. PHP does not have an extension named ext/pgsql that has been removed in the same manner as the original mysql extension; PostgreSQL support is provided by the pgsql extension, which remains a part of the PHP ecosystem. To use MySQL with PHP today, you should use either the MySQLi (MySQL Improved) extension or the PDO_MySQL (PHP Data Objects) extension [1][2]. Setup for PDO_MySQL: 1. Installation: Ensure the pdo_mysql extension is installed. On many Linux distributions, this is often included in a package named php-mysql or similar [4][5]. If you are compiling from source, use the --with-pdo-mysql flag [4][5]. 2. Connection: Use the PDO class to connect to your database. You must provide a Data Source Name (DSN) that begins with the mysql: prefix [6][7]. Example usage: $dsn = 'mysql:host=localhost;dbname=testdb;charset=utf8mb4'; $username = 'your_username'; $password = 'your_password'; try { $pdo = new PDO($dsn, $username, $password, [ PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION ]); echo "Connected successfully."; } catch (PDOException $e) { echo "Connection failed: ". $e->getMessage; } For more details, see the official PHP documentation on the PDO_MySQL driver [4] and the PDO connection page [8].
Citations:
- 1: https://www.php.net/manual/en/book.mysql.php
- 2: https://www.php.net/manual/en/mysql.requirements.php
- 3: https://stackoverflow.com/questions/13944956/the-mysql-extension-is-deprecated-and-will-be-removed-in-the-future-use-mysqli
- 4: https://www.php.net/pdo_mysql
- 5: https://www.php.net/manual/en/ref.pdo-mysql
- 6: https://www.php.net/pdo-mysql.connection
- 7: https://www.php.net/pdo
- 8: https://www.php.net/pdo.connections
Install the PDO MySQL driver explicitly.
The workflow uses driver: pdo_mysql, but requests the removed ext/mysql extension in setup-php. Use pdo_mysql and add fail-fast: true under the setup step so a driver-install failure stops the job instead of being ignored and surfacing later during Flow bootstrap.
🔧 Proposed fix
- name: Setup PHP
uses: shivammathur/setup-php@v2
with:
php-version: ${{ matrix.php-versions }}
- extensions: mbstring, xml, json, zlib, iconv, intl, mysql, pgsql, redis, memcached, memcache, apcu
+ extensions: mbstring, xml, json, zlib, iconv, intl, pdo_mysql, pgsql, redis, memcached, memcache, apcu
ini-values: date.timezone="Africa/Tunis", opcache.fast_shutdown=0, apc.enable_cli=on
+ env:
+ fail-fast: true📝 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.
| extensions: mbstring, xml, json, zlib, iconv, intl, mysql, pgsql, redis, memcached, memcache, apcu | |
| - name: Setup PHP | |
| uses: shivammathur/setup-php@v2 | |
| with: | |
| php-version: ${{ matrix.php-versions }} | |
| extensions: mbstring, xml, json, zlib, iconv, intl, pdo_mysql, pgsql, redis, memcached, memcache, apcu | |
| ini-values: date.timezone="Africa/Tunis", opcache.fast_shutdown=0, apc.enable_cli=on | |
| env: | |
| fail-fast: true |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/test.yml at line 50, Update the setup-php configuration’s
extensions list to replace the removed mysql extension with pdo_mysql, matching
the workflow’s pdo_mysql driver requirement, and add fail-fast: true under that
setup step so installation failures stop the job immediately.
Source: MCP tools
| protected function findNode(string $nodeId, string $workspace, array $dimensionsArray): ?Node | ||
| { | ||
| if ($dimensionsArray === []) { | ||
| $dimensionsArray = $this->getDefaultDimensions(); | ||
| } | ||
| $contentRepository = $this->contentRepositoryProvider->getContentRepository(); | ||
|
|
||
| $context = $this->createContentContext($workspace, $dimensionsArray); | ||
| $node = $context->getNodeByIdentifier($nodeId); | ||
|
|
||
| if ($node === null) { | ||
| try { | ||
| if ($dimensionsArray === []) { | ||
| $dimensionSpacePoint = $this->getDefaultDimensionSpacePoint($contentRepository); | ||
| } else { | ||
| $dimensionSpacePoint = DimensionSpacePoint::fromArray($this->dimensionCoordinatesFromLegacyDimensionsArray($dimensionsArray)); | ||
| } | ||
| $subgraph = $contentRepository->getContentSubgraph(WorkspaceName::fromString($workspace), $dimensionSpacePoint); | ||
| return $subgraph->findNodeById(NodeAggregateId::fromString($nodeId)); | ||
| } catch (\InvalidArgumentException | \Neos\ContentRepository\Core\SharedModel\Exception\WorkspaceDoesNotExist $e) { | ||
| return null; | ||
| } | ||
|
|
||
| $site = $this->findSiteForNode($node); | ||
| if ($site === null) { | ||
| return $node; | ||
| } | ||
|
|
||
| $contextProperties = $node->getContext()->getProperties(); | ||
| $contextProperties['currentSite'] = $site; | ||
| if ($domain = $site->getFirstActiveDomain()) { | ||
| $contextProperties['currentDomain'] = $domain; | ||
| } | ||
| $siteContext = $this->_contextFactory->create($contextProperties); | ||
|
|
||
| return $siteContext->getNodeByIdentifier($nodeId); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files matching PreviewRenderController.php and NodeVisibility.php"
fd -a 'PreviewRenderController.php|NodeVisibility\.php' . || true
echo
echo "Diff/stat context if available"
git diff --stat || true
git diff -- PreviewRenderController.php NodeVisibility.php 2>/dev/null | sed -n '1,220p' || true
echo
echo "Locate controller and helper paths"
controller=$(fd 'PreviewRenderController.php' . | head -n1 || true)
visibility=$(fd 'NodeVisibility.php' . | head -n1 || true)
echo "controller=$controller"
echo "visibility=$visibility"
if [ -n "${controller:-}" ]; then
echo
echo "Controller outline"
ast-grep outline "$controller" --view expanded || true
echo
echo "Relevant controller lines"
nl -ba "$controller" | sed -n '1,240p'
fi
if [ -n "${visibility:-}" ]; then
echo
echo "NodeVisibility helper"
nl -ba "$visibility" | sed -n '1,220p'
fi
echo
echo "Search for visibility helpers and getContentSubgraph usage"
rg -n "getContentSubgraph|getSubgraph\\(|NodeVisibility|excludeRemoved|excludeDisabledAndRemoved|AccessDenied|withoutAuthorizationChecks|getContentRepository\\(|ContentRepositoryAuthProvider::getVisibilityConstraints" . || trueRepository: NEOSidekick/NEOSidekick.AiAssistant
Length of output: 1334
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files matching PreviewRenderController.php and NodeVisibility.php"
fd -a 'PreviewRenderController.php|NodeVisibility\.php' . || true
echo
echo "Diff/stat context if available"
git diff --stat || true
git diff -- PreviewRenderController.php NodeVisibility.php 2>/dev/null | sed -n '1,220p' || true
echo
echo "Locate controller and helper paths"
controller=$(fd 'PreviewRenderController.php' . | head -n1 || true)
visibility=$(fd 'NodeVisibility.php' . | head -n1 || true)
echo "controller=$controller"
echo "visibility=$visibility"
if [ -n "${controller:-}" ]; then
echo
echo "Controller outline"
ast-grep outline "$controller" --view expanded || true
echo
echo "Relevant controller lines"
nl -ba "$controller" | sed -n '1,240p'
fi
if [ -n "${visibility:-}" ]; then
echo
echo "NodeVisibility helper"
nl -ba "$visibility" | sed -n '1,220p'
fi
echo
echo "_SEARCH_REPLACE_FAILED_Repository: NEOSidekick/NEOSidekick.AiAssistant
Length of output: 1334
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Controller lines with read-line tool"
sed -n '1,230p' Classes/Controller/PreviewRenderController.php 2>/dev/null || true
echo
echo "NodeVisibility helper"
sed -n '1,220p' Classes/Service/NodeVisibility.php 2>/dev/null ||真
echo
echo "Relevant search terms"
rg -n "getContentSubgraph|getSubgraph\\(|NodeVisibility|excludeRemoved|excludeDisabledAndRemoved|AccessDenied|withoutAuthorizationChecks|getContentRepository\\(|ContentRepositoryAuthProvider::getVisibilityConstraints|previewToken|initialize" Classes config Package.php 2>/dev/null || true
echo
echo "Composer/package constraints"
if [ -f "composer.json" ]; then
sed -n '1,220p' composer.json
fi
if [ -f "composer.lock" ]; then
python3 - <<'PY'
import json
from pathlib import Path
p=Path('composer.lock')
if p.exists():
data=json.loads(p.read_text())
for name in ['neos/contentrepository.core','neos/contentrepository','neos/neos']:
for pkg in data.get('packages', [])+data.get('packages-dev', []):
if pkg.get('name')==name:
print(name, pkg.get('version'), pkg.get('source',{}).get('type'), pkg.get('source',{}).get('url'))
PY
fiRepository: NEOSidekick/NEOSidekick.AiAssistant
Length of output: 21314
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Controller lines with read-line tool"
sed -n '1,230p' Classes/Controller/PreviewRenderController.php 2>/dev/null || true
echo
echo "NodeVisibility helper"
sed -n '1,220p' Classes/Service/NodeVisibility.php 2>/dev/null || true
echo
echo "Relevant search terms"
rg -n "getContentSubgraph|getSubgroup\\(|getSubgraph\\(|NodeVisibility|excludeRemoved|excludeDisabledAndRemoved|AccessDenied|withoutAuthorizationChecks|getContentRepository\\(|ContentRepositoryAuthProvider::getVisibilityConstraints|previewToken|initialize" Classes config Package.php 2>/dev/null || true
echo
echo "Composer/package constraints"
if [ -f "composer.json" ]; then
sed -n '1,220p' composer.json
fi
if [ -f "composer.lock" ]; then
python3 - <<'PY'
import json
from pathlib import Path
p=Path('composer.lock')
if p.exists():
data=json.loads(p.read_text())
for name in ['neosc/contentrepository.core','neos/contentrepository','neos/neos']:
for pkg in data.get('packages', [])+data.get('packages-dev', []):
if pkg.get('name')==name:
print(name, pkg.get('version'), pkg.get('source',{}).get('type'), pkg.get('source',{}).get('url'))
PY
fiRepository: NEOSidekick/NEOSidekick.AiAssistant
Length of output: 21314
🌐 Web query:
Neos 9 ContentRepository getContentSubgraph VisibilityConstraints withoutAuthorizationChecks AccessDenied
💡 Result:
In Neos 9, the ContentRepository is designed to enforce security by default, which is why attempting to retrieve a ContentSubgraph without proper authorization can lead to an AccessDenied exception [1][2]. When you call $contentRepository->getContentSubgraph, the system implicitly uses the VisibilityConstraints of the current user, which are determined by the ContentRepositoryAuthorizationService based on the user's roles [1][3]. If you need to bypass these standard authorization checks, you can use the Flow SecurityContext's ability to disable authorization checks, or specifically manipulate the VisibilityConstraints [4][5]. 1. Disabling Authorization Checks: The Neos security system allows running code in a closure where authorization checks are disabled [5]. $this->securityContext->withoutAuthorizationChecks(function use ($contentRepository, $workspaceName, $dimensionSpacePoint) { $subgraph = $contentRepository->getContentSubgraph($workspaceName, $dimensionSpacePoint); //... }); 2. Using Specific VisibilityConstraints: Alternatively, you can manually define the VisibilityConstraints when requesting the subgraph to control what content is visible, such as using VisibilityConstraints::withoutRestrictions to fetch all content regardless of user permissions [6][7]. $subgraph = $contentRepository->getContentGraph($workspaceName)->getSubgraph( $dimensionSpacePoint, VisibilityConstraints::withoutRestrictions); Note that while bypassing checks is possible, developers should be cautious to check areAuthorizationChecksDisabled in custom code if needed, as the system does not always automatically handle every edge case, especially in CLI environments where the security context might not be fully initialized [5]. An AccessDenied exception generally indicates that the requested operation or data access violates the current user's privileges as enforced by the security framework [2][8][9].
Citations:
- 1: https://neos.github.io/neos/9.0/Neos/ContentRepository/Core/ContentRepository.html
- 2: https://neos.github.io/neos/9.0/Neos/ContentRepository/Core/Feature/Security/Exception/AccessDenied.html
- 3: https://neos.github.io/neos/9.0/Neos/Neos/Security/Authorization/ContentRepositoryAuthorizationService.html
- 4: https://github.com/neos/neos-development-collection/blob/9.0/Neos.Neos/Classes/Security/ContentRepositoryAuthProvider/ContentRepositoryAuthProvider.php
- 5: https://neos.readthedocs.io/en/stable/Appendixes/ChangeLogs/908.html
- 6: https://neos.github.io/neos/9.0/Neos/ContentRepository/Core/Projection/ContentGraph/VisibilityConstraints.html
- 7: https://docs.neos.io/guide/content-repository/php-api/nodes
- 8: https://docs.neos.io/guide/content-repository/security
- 9: https://neos.github.io/flow/7.0/Neos/Flow/Security/Exception/AccessDeniedException.html
Use explicit frontend visibility when fetching preview nodes
findNode() intentionally disables authorization checks, but getContentSubgraph() still derives visibility from the auth provider. Since the package defines intent-revealing visibility helpers, use getSubgraph() with NodeVisibility::excludeRemoved() so preview behavior does not depend on Neos 9 patch/authorization implementation details.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Classes/Controller/PreviewRenderController.php` around lines 150 - 165, The
findNode() method currently fetches its subgraph through
contentRepository->getContentSubgraph(), allowing authorization-derived
visibility. Replace that fetch with the package’s getSubgraph() path and
explicitly pass NodeVisibility::excludeRemoved(), while preserving the existing
workspace, dimension-space point, node lookup, and exception handling.
| public function contentDimensionsByName(): array | ||
| { | ||
| $contentRepositoryId = $this->contentRepositoryProvider->getContentRepositoryId(); | ||
| return (new \Neos\Neos\Ui\Fusion\Helper\ContentDimensionsHelper())->contentDimensionsByName($contentRepositoryId); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the installed helper for injected dependencies
fd -t f 'ContentDimensionsHelper.php' | xargs -r sed -n '1,80p'Repository: NEOSidekick/NEOSidekick.AiAssistant
Length of output: 173
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files matching helper/eel:"
fd -t f 'NEOSidekickInternalHelper.php|ContentDimensionsHelper.php' .
echo
echo "Git status/stat:"
git diff --stat 2>/dev/null || true
git status --short 2>/dev/null || true
echo
echo "Relevant class outline/content:"
if [ -f Classes/EelHelper/NEOSidekickInternalHelper.php ]; then
wc -l Classes/EelHelper/NEOSidekickInternalHelper.php
sed -n '1,160p' Classes/EelHelper/NEOSidekickInternalHelper.php
else
echo "Classes/EelHelper/NEOSidekickInternalHelper.php not found"
fi
echo
echo "Search for ContentDimensionsDependency usage:"
rg -n "ContentDimensionsHelper|contentDimensionsByName|ContentRepositoryRegistry|contentRepositoryRegistry" . --glob '!vendor' --glob '!Build' --glob '!node_modules' || trueRepository: NEOSidekick/NEOSidekick.AiAssistant
Length of output: 10150
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Dependencies and lockfile package versions:"
fd -t f 'composer.json|composer.lock' . -x sh -c 'echo "--- $1"; sed -n "1,80p;180,260p" "$1"' sh {}
echo
echo "Check for Neos.UI package in composer.lock:"
if [ -f composer.lock ]; then
php -r '
$lock = json_decode=file_get_contents("composer.lock"), true;
foreach(($lock["packages"] ?? []) + ($lock["packages-dev"] ?? []) as $pkg) {
if ($pkg["name"] === "neos/neos" || $pkg["name"] === "neos/neos-ui") {
echo $pkg["name"] . " " . $pkg["version"] . " " . ($pkg["source"]["type"] ?? "") . " " . ($pkg["source"]["url"] ?? "") . " " . ($pkg["source"]["reference"] ?? "") . PHP_EOL;
}
}
'
else
echo "no composer.lock"
fi
echo
echo "Query package source for helper:"
if [ -f composer.lock ]; then
python3 - <<'PY'
import json, urllib.request
p=json.loads(open("composer.lock").read())
for sec in ("packages","packages-dev"):
for c in p.get(sec, []):
if c["name"] in ("neos/neos","neos/neos-ui"):
ref=c.get("source",{}).get("reference") or c.get("version","")
if c["name"]=="neos/neos-ui":
url=c.get("source",{}).get("url") or "https://api.github.com/repos/neos/neos-ui/git/trees/"+ref
else:
url=f"https://raw.githubusercontent.com/neos/neos/{ref}/Neos.Neos/Classes/Fusion/Helper/ContentDimensionsHelper.php"
print(f"--- {c['name']} {ref}")
print(url)
try:
with urllib.request.urlopen(url, timeout=20) as r:
text=r.read().decode("utf-8","replace")
for i,line in enumerate(text.splitlines(),1):
if i<=90:
print(f"{i:4}: {line}")
except Exception as e:
print(f"FETCH_ERROR: {e}")
PY
fiRepository: NEOSidekick/NEOSidekick.AiAssistant
Length of output: 2496
🌐 Web query:
Neos 9 Neos.Neos Fusion Helper ContentDimensionsHelper source contentRepositoryRegistry
💡 Result:
In Neos 9, the legacy ContentDimensionsHelper has been superseded by the new DimensionHelper (Neos\Neos\Fusion\Helper\DimensionHelper) to handle content dimensions within Fusion [1]. Key points regarding the transition and usage in Neos 9: 1. DimensionHelper: This is the new Eel helper used to interact with content dimensions [1]. You can access it in Fusion via Neos.Dimension [1]. Common methods include: - Neos.Dimension.currentValue(node, dimensionName): Retrieves the current dimension value for a node [2][1]. - Neos.Dimension.originValue(node, dimensionName): Gets the original dimension value, which may differ from the current value if dimension fallbacks are active [2][3]. - Neos.Dimension.all(node/contentRepositoryId): Finds all content dimensions defined for the given node or repository [2][1]. - Neos.Dimension.allDimensionValues(node/contentRepositoryId, dimensionName): Retrieves all dimension values for a specific dimension [2][1]. 2. ContentRepositoryRegistry: While not a Fusion helper, the ContentRepositoryRegistry (Neos\ContentRepositoryRegistry\ContentRepositoryRegistry) is the central entry point in Neos 9 for fetching Content Repository instances in PHP [4][5]. Within the DimensionHelper's implementation, the registry is used to resolve the specific Content Repository associated with a node, allowing the helper to query the appropriate variation graph [3][6]. 3. Upgrade Context: If you are migrating from Neos 8.3 to 9.0, note that the previous approach of accessing dimensions via site.context.dimensions is no longer available because the Request Context has been removed [1]. You must update your Fusion code to use the Neos.Dimension helper instead [1].
Citations:
- 1: https://docs.neos.io/api/upgrade-instructions/version-9-x/upgrade-instructions-8-3-9-0/fusion-api
- 2: https://neos.github.io/neos/9.0/Neos/Neos/Fusion/Helper/DimensionHelper.html
- 3: https://github.com/neos/neos-development-collection/blob/9.0/Neos.Neos/Classes/Fusion/Helper/DimensionHelper.php
- 4: https://neos.github.io/neos/9.0/Neos/ContentRepositoryRegistry/ContentRepositoryRegistry.html
- 5: https://docs.neos.io/guide/content-repository/php-api
- 6: https://github.com/neos/neos-development-collection/blob/9.0/Neos.Neos/Classes/Controller/Module/Administration/DimensionController.php
🌐 Web query:
github.com neos neos-development-collection 9.0 Neos.News.Fusion.Helper.ContentDimensionsHelper.php
💡 Result:
In Neos 9.0, there is no file named ContentDimensionsHelper.php in the Neos.News.Fusion (or Neos.Neos) package [1][2][3][4]. It appears you are likely looking for the Neos.Dimension helper, which was introduced in Neos 9.0 to handle content dimension-related tasks in Fusion [5][6]. The class responsible for this functionality is Neos\Neos\Fusion\Helper\DimensionHelper [1][2]. You can find this helper at the following location in the neos-development-collection repository: Neos.Neos/Classes/Fusion/Helper/DimensionHelper.php [1] This helper provides several methods for working with node dimensions in Fusion, including: - currentValue(Node $node, ContentDimensionId|string $dimensionName): Retrieves the current dimension value for a node [2][5]. - originValue(Node $node, ContentDimensionId|string $dimensionName): Retrieves the original dimension value, differing from the current value in cases of dimension fallback [2][5]. - all(ContentRepositoryId|Node $subject): Finds all content dimensions in a repository [2][5]. - allDimensionValues(ContentRepositoryId|Node $subject, ContentDimensionId|string $dimensionName): Finds all content dimension values [2][5]. - findVariantInDimension(Node $node, ContentDimensionId|string $dimensionName, ContentDimensionValue|string $dimensionValue): Finds the variant of a node in a specified dimension and value [2][5].
Citations:
- 1: https://github.com/neos/neos-development-collection/blob/9.0/Neos.Neos/Classes/Fusion/Helper/DimensionHelper.php
- 2: https://neos.github.io/neos/9.0/Neos/Neos/Fusion/Helper/DimensionHelper.html
- 3: https://neos.github.io/neos/9.0/Neos/Neos/Fusion/DimensionsMenuItemsImplementation.html
- 4: https://neos.github.io/neos/9.0/Neos/Neos/Fusion.html
- 5: FEATURE: Add
Neos.Dimensionhelper neos/neos-development-collection#4085 - 6: https://github.com/neos/neos-development-collection/releases/tag/9.0.0
🌐 Web query:
github.com neos/neos-ui 9.0 Fusion/Helper/ContentDimensionsHelper.php
💡 Result:
The file ContentDimensionsHelper.php, located in the Neos UI package (neos/neos-ui), is an Eel helper class responsible for handling content dimension configurations within the Neos CMS user interface [1][2]. In older versions of Neos (such as 7.1.x), this helper was explicitly documented as Neos\Neos\Ui\Fusion\Helper\ContentDimensionsHelper and provided Eel methods like ContentDimensions.allowedPresetsByName and ContentDimensions.contentDimensionsByName [2]. In Neos 9.0 and later versions, the core handling of content dimensions and node variants has been significantly refactored alongside the Content Repository (CR) updates [3][4]. Functionality previously handled by various UI-specific helpers has been migrated into the core Neos namespace [5][4]. For instance, current dimension management in Fusion is primarily handled by the Neos\Neos\Fusion\Helper\DimensionHelper [5][4]. If you are working with Neos 9.0, you should look for updated Fusion helpers within the Neos\Neos\Fusion\Helper namespace rather than relying on legacy UI-specific helpers [5][4].
Citations:
- 1: https://il.kronospan-express.com/mystyle-flooring.com/Packages/Application/Neos.Neos.Ui/Classes/Fusion/Helper/
- 2: https://neos.readthedocs.io/en/7.1/References/EelHelpersReference.html
- 3: https://docs.neos.io/guide/essentials/content-repository/dimensions
- 4: https://github.com/neos/neos-development-collection/blob/9.0/Neos.Neos/Classes/Fusion/Helper/DimensionHelper.php
- 5: https://neos.github.io/neos/9.0/Neos/Neos/Fusion/Helper/DimensionHelper.html
🌐 Web query:
siteground Kronospan neos neos-ui 9.0 ContentDimensionsByName ContentDimensionsHelper
💡 Result:
In Neos CMS 9.0, which utilizes the new event-sourced Content Repository, the legacy Eel helpers such as ContentDimensionsHelper and methods like ContentDimensionsByName have been replaced or reorganized [1][2]. For accessing dimension information in Fusion, Neos 9.0 provides the Neos.Dimension helper, which is implemented in the class Neos\Neos\Fusion\Helper\DimensionHelper [3][4]. This helper allows you to interact with content dimensions and their values for a given node or content repository ID [3][4]. Key methods available in the Neos.Dimension (DimensionHelper) include: 1. Neos.Dimension.all(node) or Neos.Dimension.all(contentRepositoryId): Returns all content dimensions defined in the content repository, ordered by priority [3][4]. 2. Neos.Dimension.allDimensionValues(node, dimensionName): Retrieves all content dimension values for a specific dimension [3][4]. 3. Neos.Dimension.currentValue(node, dimensionName): Returns the current dimension value for a specific node [3]. 4. Neos.Dimension.findVariantInDimension(node, dimensionName, dimensionValue): Finds a specific variant of a node within a given dimension and value [3]. The legacy class Neos\Neos\Ui\Fusion\Helper\ContentDimensionsHelper and its associated Eel methods (like contentDimensionsByName) are associated with older versions of Neos (e.g., 7.3.x) and are not the standard approach in the 9.0 architecture [5]. If you are migrating to or developing on Neos 9.0, you should update your Fusion code to utilize the Neos.Dimension helper [3].
Citations:
- 1: https://docs.neos.io/guide/essentials/content-repository/dimensions
- 2: https://docs.neos.io/guide/content-repository/content-dimensions
- 3: https://neos.github.io/neos/9.0/Neos/Neos/Fusion/Helper/DimensionHelper.html
- 4: https://github.com/neos/neos-development-collection/blob/9.0/Neos.Neos/Classes/Fusion/Helper/DimensionHelper.php
- 5: https://neos.readthedocs.io/en/7.3/References/EelHelpersReference.html
🌐 Web query:
raw.githubusercontent.com neos neos-ui 9.0 Neos.Neos.Ui Classes Fusion Helper ContentDimensionsHelper.php
💡 Result:
In Neos 9.0, the class Neos\Neos\Ui\Fusion\Helper\ContentDimensionsHelper is associated with the Neos UI package, but official documentation and the 9.0 source code indicate that content dimension handling has evolved significantly in this version [1][2]. While documentation for Neos 7.3 references ContentDimensionsHelper to provide methods such as allowedPresetsByName and contentDimensionsByName [3], Neos 9.0 introduces a new content repository architecture (often referred to as the "CR Core") [1]. In the 9.0 API, functionality related to content dimensions has shifted toward the Neos\Neos\Fusion\Helper\DimensionHelper class [4]. DimensionHelper provides several methods for managing nodes across dimensions, including: - all(ContentRepositoryId|Node $subject): Finds all content dimensions in a repository or for a specific node [4]. - currentValue(Node $node, ContentDimensionId|string $dimensionName): Retrieves the current dimension value for a node [4]. - originValue(Node $node, ContentDimensionId|string $dimensionName): Retrieves the original dimension value, accounting for dimension fallbacks [4]. - findVariantInDimension(Node $node, ContentDimensionId|string $dimensionName, ContentDimensionValue|string $dimensionValue): Locates a node variant in a specific dimension [4]. If you are upgrading from older versions, check the Neos 9.0 documentation and the new DimensionHelper class, as direct reliance on ContentDimensionsHelper may be deprecated or replaced by the updated CR infrastructure [1][4].
Citations:
- 1: https://github.com/neos/neos-development-collection/blob/9.0/Neos.ContentRepository.Core/Classes/Dimension/ContentDimension.php
- 2: https://github.com/neos/neos-development-collection/blob/9.0/Neos.ContentRepositoryRegistry/Classes/Factory/ContentDimensionSource/ConfigurationBasedContentDimensionSourceFactory.php
- 3: https://neos.readthedocs.io/en/7.3/References/EelHelpersReference.html
- 4: https://neos.github.io/neos/9.0/Neos/Neos/Fusion/Helper/DimensionHelper.html
🌐 Web query:
github.com neos/neos-ui master Neos.Neos.Ui Classes Fusion Helper ContentDimensionsHelper.php contentDimensionsByName
💡 Result:
In Neos CMS, the functionality for handling content dimensions in Fusion is primarily provided by the DimensionHelper, which is registered as the Neos.Dimension Eel helper [1]. There is no public method named contentDimensionsByName in the standard Neos.Dimension helper or the associated DimensionHelper.php class [2][3][1]. Instead, to work with content dimensions in Fusion, the Neos.Dimension helper provides the following methods [3][1]: 1. Neos.Dimension.all(subject): Retrieves all content dimensions for a given node or content repository ID [2][3][1]. 2. Neos.Dimension.allDimensionValues(subject, dimensionName): Finds all available values for a specific content dimension [2][3][1]. 3. Neos.Dimension.currentValue(node, dimensionName): Gets the current dimension value for a specific node [3][1]. 4. Neos.Dimension.originValue(node, dimensionName): Gets the original (non-fallback) dimension value for a node [3][1]. 5. Neos.Dimension.findVariantInDimension(node, dimensionName, dimensionValue): Finds a specific variant of a node within a given dimension and value [3][1]. If you are looking to retrieve a specific dimension definition by its name (ID), you would typically interact with the ContentDimensionSource, which is accessible via the underlying Neos Content Repository API rather than a direct Fusion helper method [4].
Citations:
- 1: https://neos.readthedocs.io/en/stable/References/EelHelpersReference.html
- 2: https://github.com/neos/neos-development-collection/blob/9.0/Neos.Neos/Classes/Fusion/Helper/DimensionHelper.php
- 3: https://neos.github.io/neos/9.0/Neos/Neos/Fusion/Helper/DimensionHelper.html
- 4: https://neos.github.io/neos/9.0/Neos/ContentRepository/Core/Dimension/ContentDimensionSourceInterface.html
Replace the legacy Neos UI content dimensions helper.
This package targets Neos 9, where Neos\Neos\Ui\Fusion\Helper\ContentDimensionsHelper::contentDimensionsByName() is no longer the supported API; use Neos\Neos\Fusion\Helper\DimensionHelper / the Neos.Dimension Eel helper equivalents instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Classes/EelHelper/NEOSidekickInternalHelper.php` around lines 100 - 104,
Update contentDimensionsByName() to stop instantiating the legacy
Neos\Neos\Ui\Fusion\Helper\ContentDimensionsHelper and retrieve the dimensions
through the supported Neos\Neos\Fusion\Helper\DimensionHelper or equivalent
Neos.Dimension Eel helper API, preserving the existing content repository
context and array result.
| if ($hidden === true) { | ||
| $contentRepository->handle(DisableNodeAggregate::create( | ||
| $subgraph->getWorkspaceName(), | ||
| $newNodeAggregateId, | ||
| $subgraph->getDimensionSpacePoint(), | ||
| NodeVariantSelectionStrategy::STRATEGY_ALL_SPECIALIZATIONS | ||
| )); | ||
| } | ||
|
|
||
| // The projection is updated synchronously, so the new node is queryable right away | ||
| $newNode = $subgraph->findNodeById($newNodeAggregateId); | ||
| if ($newNode === null) { | ||
| throw new PatchFailedException( | ||
| sprintf('Node "%s" was created but cannot be found in the subgraph', $newNodeAggregateId->value), | ||
| $index, | ||
| 'createNode', | ||
| $patch->getPositionRelativeToNodeId() | ||
| ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Read the created node before disabling it.
If the resolved subgraph applies any visibility constraint, findNodeById() after DisableNodeAggregate returns null and every hidden-node creation fails with "created but cannot be found" after the node has already been created (no rollback in the event-sourced CR). Reordering removes the dependency on the subgraph's visibility semantics entirely.
🐛 Proposed fix
- if ($hidden === true) {
- $contentRepository->handle(DisableNodeAggregate::create(
- $subgraph->getWorkspaceName(),
- $newNodeAggregateId,
- $subgraph->getDimensionSpacePoint(),
- NodeVariantSelectionStrategy::STRATEGY_ALL_SPECIALIZATIONS
- ));
- }
-
// The projection is updated synchronously, so the new node is queryable right away
$newNode = $subgraph->findNodeById($newNodeAggregateId);
if ($newNode === null) {
throw new PatchFailedException(
sprintf('Node "%s" was created but cannot be found in the subgraph', $newNodeAggregateId->value),
$index,
'createNode',
$patch->getPositionRelativeToNodeId()
);
}
+
+ if ($hidden === true) {
+ $contentRepository->handle(DisableNodeAggregate::create(
+ $subgraph->getWorkspaceName(),
+ $newNodeAggregateId,
+ $subgraph->getDimensionSpacePoint(),
+ NodeVariantSelectionStrategy::STRATEGY_ALL_SPECIALIZATIONS
+ ));
+ }📝 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.
| if ($hidden === true) { | |
| $contentRepository->handle(DisableNodeAggregate::create( | |
| $subgraph->getWorkspaceName(), | |
| $newNodeAggregateId, | |
| $subgraph->getDimensionSpacePoint(), | |
| NodeVariantSelectionStrategy::STRATEGY_ALL_SPECIALIZATIONS | |
| )); | |
| } | |
| // The projection is updated synchronously, so the new node is queryable right away | |
| $newNode = $subgraph->findNodeById($newNodeAggregateId); | |
| if ($newNode === null) { | |
| throw new PatchFailedException( | |
| sprintf('Node "%s" was created but cannot be found in the subgraph', $newNodeAggregateId->value), | |
| $index, | |
| 'createNode', | |
| $patch->getPositionRelativeToNodeId() | |
| ); | |
| } | |
| // The projection is updated synchronously, so the new node is queryable right away | |
| $newNode = $subgraph->findNodeById($newNodeAggregateId); | |
| if ($newNode === null) { | |
| throw new PatchFailedException( | |
| sprintf('Node "%s" was created but cannot be found in the subgraph', $newNodeAggregateId->value), | |
| $index, | |
| 'createNode', | |
| $patch->getPositionRelativeToNodeId() | |
| ); | |
| } | |
| if ($hidden === true) { | |
| $contentRepository->handle(DisableNodeAggregate::create( | |
| $subgraph->getWorkspaceName(), | |
| $newNodeAggregateId, | |
| $subgraph->getDimensionSpacePoint(), | |
| NodeVariantSelectionStrategy::STRATEGY_ALL_SPECIALIZATIONS | |
| )); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Classes/Service/NodePatchService.php` around lines 357 - 375, Reorder the
hidden-node creation flow so `$subgraph->findNodeById($newNodeAggregateId)` and
its null check occur before dispatching `DisableNodeAggregate`. Keep the
existing disable operation afterward, ensuring hidden nodes are validated while
visible and avoiding dependence on subgraph visibility constraints.
| // Remember which asset is listed first (persistence check below is identity-based: | ||
| // page counts are unreliable because the module refills pages from the asset pool) | ||
| const firstImageSrc = await page.locator('#appContainer img').first().getAttribute('src'); | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the test file and relevant test block.
fd -a 'neos9.bulkAltTextModule.spec.ts' . | sed 's#^\./##'
file="$(fd 'neos9.bulkAltTextModule.spec.ts' . | head -n1)"
echo "FILE=$file"
wc -l "$file"
echo "==== lines 1-120 ===="
sed -n '1,120p' "$file" | cat -n
echo "==== occurrences of firstImageSrc and related assertions ===="
rg -n "firstImageSrc|asset|title|persisted|isVisible|locator\\('`#appContainer` img'\\)|`#appContainer` img|first()" "$file"Repository: NEOSidekick/NEOSidekick.AiAssistant
Length of output: 7145
🌐 Web query:
Playwright Locator click button Start generation Generierung starten waitForTest 106 documentation
💡 Result:
In Playwright, there is no built-in method named waitForTest. It is likely a misunderstanding of testInfo or a custom helper function used in specific test suites [1][2]. To interact with a button labeled "Start generation" (or "Generierung starten"), use Playwright's locator API, which is designed to handle synchronization automatically [3][4]. You should use getByRole to target the button [5]. Recommended implementation: // TypeScript/JavaScript await page.getByRole('button', { name: 'Start generation' }).click; // Or for German locale: await page.getByRole('button', { name: 'Generierung starten' }).click; Key concepts for this task: 1. Locator Auto-waiting: You do not need to manually wait for the button to appear [3]. The click() action automatically performs actionability checks—including waiting for the element to be visible, stable (not animating), enabled, and receiving pointer events—before executing the click [6][4]. 2. Test Generator (Codegen): If you are struggling to identify the correct selector, you can use Playwright's built-in test generator by running npx playwright codegen <your-url>. This tool allows you to interact with the page and automatically generates the recommended locators, prioritizing robust options like getByRole [7][8]. 3. Handling Timeouts: If your test times out, it is often because the element is not meeting actionability criteria (e.g., it is obscured or disabled). You can verify the element's state or, in exceptional cases, use the force: true option to bypass these checks: await page.getByRole('button', { name: 'Start generation' }).click({ force: true }); [6][4] If you encountered waitForTest in a codebase, please check if it is defined as a custom utility function in your project, as it is not part of the official Playwright API [1]. If you are looking to manage test retries or information, refer to the testInfo object instead [1][2].
Citations:
- 1: https://playwrightsolutions.com/how-to-use-playwrights-testinforetry-to-deal-with-flakey-environments/
- 2: https://playwright.help/docs/api/class-testinfo
- 3: https://playwright.help/docs/writing-tests
- 4: https://playwright.dev/docs/actionability
- 5: https://playwright.dev/docs/locators
- 6: https://playwright.dev/docs/input
- 7: https://playwright.dev/docs/next/codegen
- 8: https://github.com/microsoft/playwright/blob/main/docs/src/codegen-intro.md
🌐 Web query:
Playwright Locator isVisible return boolean timeout option documented
💡 Result:
The Playwright locator.isVisible method returns a boolean value (true if the element is visible, false otherwise) [1][2][3]. While some language bindings may still include a timeout option in their API signature for backward compatibility, it is officially deprecated [2][4][5]. The timeout option is ignored because locator.isVisible is designed to perform an immediate check without waiting for the element to become visible [1][2][5][6]. If you need to wait for an element to become visible, you should use the Web-First assertion expect(locator).toBeVisible instead, which includes built-in retry logic and configurable timeouts [2][3][6].
Citations:
- 1: https://playwright.dev/docs/api/class-locator
- 2: [Feature]: remove the deprecated
timeoutargment toLocator.isVisible, or make the deprecation more obvious to the user microsoft/playwright#33017 - 3: https://playwright.dev/java/docs/api/class-locator
- 4: [Question] why was
isVisibleandisHidden'stimeoutdeprecated? microsoft/playwright#12672 - 5: https://javadoc.io/static/com.microsoft.playwright/playwright/1.53.0/com/microsoft/playwright/Locator.IsVisibleOptions.html
- 6: [BUG]: isVisible() is not working to check if the locator is present on the page or not microsoft/playwright#20685
Assert the saved asset by identity and wait for the post-save state.
isVisible({ timeout: 60_000 }) returns immediately, so this can compare against state before the post-save list has rendered. The first image source also doesn’t identify the saved asset; ordering or pagination changes can make another asset appear first. Capture a stable asset/key identifier, wait for the list empty state or the post-save list via an explicit assertion, then assert that specific asset no longer appears or its persisted title is present.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Tests/E2E/tests/neos9.bulkAltTextModule.spec.ts` around lines 36 - 39, Update
the persistence check around firstImageSrc to capture a stable identifier for
the asset being edited instead of relying on the first image source. After
saving, explicitly wait for the post-save list state using an assertion for the
empty state or rendered list, then verify that specific asset is absent or its
persisted title is present; do not use immediate isVisible checks or list
ordering as the synchronization/identity mechanism.
Source: MCP tools
| // Step 4: save (persists via the updateAssets endpoint) | ||
| const saveAll = page.getByRole('button', { name: /Save all and get next page|Alle speichern/i }).first(); | ||
| await expect(saveAll).toBeVisible({ timeout: 30_000 }); | ||
| await saveAll.click(); | ||
| console.log('[e2e] save clicked'); | ||
|
|
||
| // Saving either advances to the next page or shows the saved state; there must be no alert | ||
| await page.waitForTimeout(5_000); | ||
| await expect(page.locator('#appContainer [role="alert"], #appContainer .neos-error')).toHaveCount(0); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate target file =="
fd -a 'neos9.bulkAltTextModule.spec.ts' . || true
echo "== inspect target file =="
target="$(fd 'neos9.bulkAltTextModule.spec.ts' . | head -n 1)"
if [ -n "${target:-}" ]; then
wc -l "$target"
sed -n '1,120p' "$target" | cat -n
fi
echo "== search for updateAssets / save flow =="
rg -n "updateAssets|Save all|Alle speichern|waitForTimeout|afterPageReload|reload|toBeVisible\\(.*timeout" . \
-g '!node_modules' -g '!dist' -g '!build' || true
echo "== package playwright versions =="
for f in package.json pnpm-lock.yaml package-lock.json yarn.lock bun.lockb; do
[ -f "$f" ] && { echo "--- $f"; case "$f" in package.json) jq -r '.dependencies, .devDependencies' "$f" | rg -n "playwright|`@playwright/test`" || true;; *) rg -n "`@playwright/test`|playwright" "$f" | head -n 40 || true;; esac; }
doneRepository: NEOSidekick/NEOSidekick.AiAssistant
Length of output: 50392
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== list e2e tests under Tests/E2E/tests =="
find Tests/E2E/tests -maxdepth 1 -type f -name '*.spec.ts' 2>/dev/null | sort | head -n 80
echo "== inspect target file with wc if exists =="
target="Tests/E2E/tests/neos9.bulkAltTextModule.spec.ts"
if [ -f "$target" ]; then
wc -l "$target"
sed -n '1,130p' "$target" | cat -n
else
echo "target file not found"
fi
echo "== focused rg for relevant implementation/API terms =="
rg -n "updateAssets|saveAll|Save all|Alle speichern|waitForTimeout|after reload|Reload|reload|waitForResponse|response|status" \
Tests/E2E src . \
-g '!node_modules' -g '!dist' -g '!build' -g '!coverage' --max-count 120 || true
echo "== package manifests for playwright and repo root =="
find . -maxdepth 2 -type f \( -name 'package.json' -o -name 'pnpm-lock.yaml' -o -name 'package-lock.json' -o -name 'yarn.lock' \) -print | sort | while read -r f; do
echo "--- $f"
rg -n "playwright|`@playwright/test`" "$f" | head -n 40 || true
doneRepository: NEOSidekick/NEOSidekick.AiAssistant
Length of output: 50392
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== focused file discovery =="
git ls-files | rg '(^|/)neos9\.bulkAltTextModule\.spec\.ts$|package\.json$|pnpm-lock\.yaml$|package-lock\.json$|yarn\.lock$' || true
echo "== target spec relevant lines if present =="
target="Tests/E2E/tests/neos9.bulkAltTextModule.spec.ts"
if [ -f "$target" ]; then
sed -n '1,140p' "$target" | cat -n
echo "== updateAssets usages in target =="
rg -n "updateAssets|waitForResponse|response\\.status\\(|toBeVisible|toHaveCount|timeout" "$target" || true
else
echo "target missing"
fi
echo "== focused updateAssets implementation search =="
rg -n "updateAssets|update.*asset|asset.*save|saveAll|after.*reload|reload.*asset|waitForResponse" \
Tests/E2E src . \
-g '!node_modules' -g '!dist' -g '!build' -g '!coverage' --max-count 160 || true
echo "== package playwright declarations =="
for f in package.json pnpm-lock.yaml package-lock.json yarn.lock; do
if [ -f "$f" ]; then
echo "--- $f"
rg -n "playwright|`@playwright/test`" "$f" | head -n 80 || true
fi
doneRepository: NEOSidekick/NEOSidekick.AiAssistant
Length of output: 50392
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repository top-level =="
pwd
git rev-parse --show-toplevel
git ls-files | head -n 120
echo "== target spec exact =="
if [ -f Tests/E2E/tests/neos9.bulkAltTextModule.spec.ts ]; then
wc -l Tests/E2E/tests/neos9.bulkAltTextModule.spec.ts
sed -n '1,140p' Tests/E2E/tests/neos9.bulkAltTextModule.spec.ts | cat -n
else
echo "missing Tests/E2E/tests/neos9.bulkAltTextModule.spec.ts"
fi
echo "== exact updateAssets matches in tracked files =="
git lfs ls-files 2>/dev/null || true
git ls-files | xargs rg -n "updateAssets" || true
echo "== exact E2E save/wait references in tracked files =="
rg -n "waitForTimeout\\(5_000\\)|afterPageReload|after reload|afterReload|waitForResponse|response\\.status\\(.*updateAssets|persistItems|Save all and get next page|Alle speichern" \
Tests/E2E src . \
--glob '!node_modules' --glob '!dist' --glob '!build' --glob '!coverage' --glob '!*.lock' || trueRepository: NEOSidekick/NEOSidekick.AiAssistant
Length of output: 50392
Wait for save completion instead of sleeping for five seconds.
waitForTimeout(5_000) can expire before the save completes, so the subsequent reload may observe stale state and make this test flaky. Replace the fixed delay with a reaction to the save response/confirmation or the next-page/saved-state UI before asserting errors and reloading.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Tests/E2E/tests/neos9.bulkAltTextModule.spec.ts` around lines 47 - 55,
Replace the fixed page.waitForTimeout(5_000) after saveAll.click() with an
explicit wait for the save completion signal, such as the updateAssets response,
confirmation UI, next-page navigation, or saved-state UI. Keep the existing
no-alert assertion and ensure they run only after the save has completed.
Source: MCP tools
| if (await continueBtn.isVisible({ timeout: 10_000 }).catch(() => false)) { | ||
| console.log('[e2e] clicking interstitial'); | ||
| await continueBtn.click(); | ||
| } | ||
|
|
||
| const authorizeBtn = popup.getByRole('button', { name: /^Authorize$/i }); | ||
| await expect(authorizeBtn).toBeVisible({ timeout: 15_000 }); | ||
| await authorizeBtn.click(); | ||
| console.log('[e2e] authorize clicked'); | ||
|
|
||
| // The popup shows "Authorization completed" and may close itself once the chat | ||
| // picks up the token — don't fail if it's already gone. | ||
| const completed = await popup | ||
| .getByText(/Authorization completed/i) | ||
| .isVisible({ timeout: 15_000 }) | ||
| .catch(() => false); | ||
| console.log('[e2e] authorization completed page shown:', completed); | ||
| await popup.close().catch(() => undefined); | ||
| } | ||
|
|
||
| // The chat should now render its input (it reloads itself after the popup completes; | ||
| // fall back to one page reload if it doesn't react within 30s). | ||
| const chatInput = () => page | ||
| .frameLocator('iframe[src*="api.neosidekick"]') | ||
| .locator('textarea, [contenteditable="true"], input[type="text"]') | ||
| .first(); | ||
| const ready = await chatInput().isVisible({ timeout: 30_000 }).catch(() => false); | ||
| console.log('[e2e] chat ready without reload:', ready); | ||
| if (!ready) { | ||
| await page.reload(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Playwright locator.isVisible timeout option ignored non-retrying
💡 Result:
In Playwright, the timeout option in locator.isVisible() is ignored because the method is designed as a "one-shot" operation that returns immediately [1][2][3]. It does not perform any waiting or retrying to determine the visibility of an element [2][4][3]. Key technical points regarding this behavior: - No Retrying: Unlike Playwright actions (e.g., .click()) or web-first assertions (e.g., expect(locator).toBeVisible()), isVisible() does not wait for the element to appear in the DOM or become visible [4][3][5]. It simply checks the current state at the exact moment the call is made [1][2]. - Deprecated Option: The timeout option in isVisible() has been deprecated for a significant period [6][7]. Recent updates have moved to prevent its use by typing it as never in TypeScript definitions to signal that it is a no-op [1][6]. - Recommended Alternatives: - For asserting visibility with a timeout (which includes automatic retries), use the web-first assertion: await expect(locator).toBeVisible({ timeout: 5000 }); [8][5]. - If you need to wait for an element to appear without asserting/failing the test, use await locator.waitFor({ state: 'visible', timeout: 5000 }); [9]. Note that if the element does not appear within the specified timeout, waitFor() will throw an error, whereas isVisible() would simply return false [9]. Developers are encouraged to use these alternatives to avoid flaky tests caused by conditional logic that relies on immediate state checks [4][3][5].
Citations:
- 1: fix(api): remove deprecated timeout option from isVisible and isHidden microsoft/playwright#39924
- 2: https://javadoc.io/static/com.microsoft.playwright/playwright/1.53.0/com/microsoft/playwright/Locator.IsVisibleOptions.html
- 3: https://playwright.dev/docs/api/class-locator
- 4: https://playwright.dev/docs/next/api/class-locator
- 5: [Question] Why does .isVisible() return immediately? microsoft/playwright#19396
- 6: [Feature]: remove the deprecated
timeoutargment toLocator.isVisible, or make the deprecation more obvious to the user microsoft/playwright#33017 - 7: [Question] why was
isVisibleandisHidden'stimeoutdeprecated? microsoft/playwright#12672 - 8: https://stackoverflow.com/questions/78640466/playwright-isvisible-function-is-not-working
- 9: [Bug]: isVisible() behaviour does not match Documentation, conditional test paths truthy/falsely microsoft/playwright#30230
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate package files =="
fd -a 'package\.json$' . | sed 's#^\./##' || true
echo "== Playwright versions =="
for f in $(fd 'package\.json$' .); do
echo "-- $f --"
jq -r '(.dependencies.playwright?, .devDependencies.playwright?, .dependencies["`@playwright/test`"]?, .devDependencies["`@playwright/test`"]?) // empty' "$f" || true
done
echo "== target snippet context =="
if [ -f "Tests/E2E/tests/neos9.editorsAndChat.spec.ts" ]; then
nl -ba "Tests/E2E/tests/neos9.editorsAndChat.spec.ts" | sed -n '60,110p'
else
echo "target file not found"
fi
echo "== isVisible timeout usages =="
rg -n "isVisible\(\s*\{\s*timeout|getByText\(.*\.isVisible\(\{ timeout|chatInput\(\)\.isVisible\(\{ timeout" Tests/E2E/tests/neos9.editorsAndChat.spec.ts .Repository: NEOSidekick/NEOSidekick.AiAssistant
Length of output: 619
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target file line count and relevant section =="
if [ -f "Tests/E2E/tests/neos9.editorsAndChat.spec.ts" ]; then
awk '{printf "%6d %s\n", NR, $0}' "Tests/E2E/tests/neos9.editorsAndChat.spec.ts" | sed -n '60,110p'
else
echo "target file not found"
fi
echo "== isVisible timeout usages in target =="
grep -n "isVisible" "Tests/E2E/tests/neos9.editorsAndChat.spec.ts" || true
echo "== Playwright timeout type evidence from installed/type definitions if present =="
f="Tests/E2E/package.json"
if [ -f "$f" ]; then
sed -n '1,120p' "$f"
fiRepository: NEOSidekick/NEOSidekick.AiAssistant
Length of output: 3705
Replace isVisible() with retry-based waits.
locator.isVisible({ timeout }) is a one-shot visibility check and does not wait for the element to appear, so the chat-readiness gate rejects before the 30s timeout and makes the page.reload() fallback fire much more often than needed. Use waitFor({ state: 'visible', timeout: 30_000 }) or expect(...).toBeVisible(...) with a catch wrapper instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Tests/E2E/tests/neos9.editorsAndChat.spec.ts` around lines 74 - 104, The
chat-readiness check using chatInput().isVisible({ timeout: 30_000 }) is a
one-shot check and should use a retry-based wait instead. Update the chat input
readiness flow to call waitFor({ state: 'visible', timeout: 30_000 }) or
expect(...).toBeVisible(...) with the existing catch-to-false behavior, while
preserving the page.reload() fallback when the wait fails.
- neos/neos, neos/media, neos/neos-ui: ~8.3 -> ^9.0 - neos/seo: * -> ^4.2 (first Neos 9 release) - flowpack/nodetemplates: ^2.x -> ^3.0 (Neos 9 release) - sitegeist/fusionlinkprototypes removed: no Neos 9 support upstream; core Neos.Fusion:ActionUri replaces it (Fusion adjusted in a later commit)
bin/rector with NeosRectorSets::CONTENTREPOSITORY_9_0 on Classes/ and Tests/. 22 files changed, purely automated output, no manual edits. 65 'TODO 9.0 migration' comments left by rector mark spots needing manual rewrite (LegacyContextStub usages, removed NodeData/Context APIs, commands instead of setters).
…d config
Rewrites everything the automated rector pass could not handle:
- Node queries: NodeData/Doctrine query builders replaced with ContentGraph
subgraph queries (findDescendantNodes/findChildNodes/findClosestNode) across
all dimension space points; workspace-chain reduction dropped (subgraphs
already materialize base-workspace content); hidden checks via subtree tags.
- Node identity towards the JS frontend: legacy context paths replaced with
NodeAddress JSON (opaque, round-tripped); services accept both NodeAddress
JSON and plain aggregate ids.
- Mutations: SetNodeProperties / SetNodeReferences / CreateNodeAggregateWithNode
(incl. Flowpack.NodeTemplates 3.0 handler) / Move/Remove/Disable/Enable
commands. Patch application is no longer transactional (event-sourced CR has
no rollback); all patches are pre-validated instead, dryRun stops after
validation.
- URI building: LinkingService replaced with NodeUriBuilder(+Factory); public
URI -> node resolution via EventSourcedFrontendNodeRoutePartHandler with
SiteDetectionResult route parameters.
- Dimensions: removed Neos.ContentRepository.contentDimensions config reads;
ContentDimensionSource everywhere; uriSegment mapping now read from the
site's dimension resolver options.
- PreviewRenderController: site/context rebuilding removed (FusionView resolves
site itself); NodeTypeManager now per content repository; Core NodeType API
(->name->value, nullable getNodeType()).
- AssetService: Flow 9 removed Repository::iterate() -> Doctrine toIterable().
- Fusion: Sitegeist.FusionLinkPrototypes replaced with Neos.Fusion:ActionUri on
the main request (ModuleUri equivalent verified against Neos 9 MenuHelper);
Neos.Ui.Workspace.getPersonalWorkspace(contentRepositoryId) with
allowedTargetWorkspaces from its return value.
- frontendConfiguration contentDimensions now served by our own Eel helper
(core helper requires a ContentRepositoryId not available in that context).
Single-CR assumption ('default') kept with rector-style TODOs. Semantic
differences are flagged inline as 'TODO 9.0 migration (manual)'.
Unit tests: 19/19 green. flow core:compile clean on Neos 9.1.6.
…Fusion Neos 9 registers the UI helpers only in Neos.Neos.Ui.configurationDefaultEelContext, no longer in Neos.Fusion.defaultContext, so getPersonalWorkspace() silently evaluated to null in our module Fusion and Array.set() fataled. Also guards the workspaces map against a missing personal workspace.
…ation) Runs against the Neos.Demo-based Neos 9 project (English backend UI): - focus keyword: 'Calculate with Sidekick' produces real-API suggestions, applying one fills the editor input - sidebar chat: consent popup flow (interstitial -> Authorize -> completed) and chat becomes ready; tolerates an already-authorized state Docks the sidebar via localStorage init script: in fresh headless profiles the fullscreen state would overlay the inspector.
…ibility helper VisibilityConstraints::default()/withoutRestrictions() are deprecated since 9.0 beta 19 and will be removed in Neos 10. New NodeVisibility helper maps intent: excludeDisabledAndRemoved() = old default context, excludeRemoved() = old invisibleContentShown context; built on NeosVisibilityConstraints.
…sitory FunctionalTestCase rewrite: - CR setup via ContentRepositoryMaintainer per test (Flow's testable persistence drops the non-ORM cr_* tables when compiling the test schema) + prune for isolation; live workspace, Neos.Neos:Sites root and site nodes via commands. - Content creation via CreateNodeAggregateWithNode/SetNodeProperties/ CreateNodeVariant/TagSubtree/RemoveNodeAggregate/PublishWorkspace helpers. - Template methods setUpContentInLive()/setUpContentInUserWorkspace(): user workspaces fork their base content stream at creation, so live content must exist before the workspaces are created (unlike Neos 8 context views). - Assertions on NodeAddress JSON keys (addressForPath) instead of context paths. - Mock ControllerContexts now carry a SiteDetectionResult route parameter (required by NodeUriBuilder in Neos 9). - Test languages are the distribution's dimension values de/en_US: Neos 9 validates the URI segment mapping against the dimension source and Flow's config merge cannot cleanly replace the distribution's mapping list. - Testing:HomePage is now a Neos.Neos:Site subtype (children of Neos.Neos:Sites must be sites in Neos 9). - AssetUsage + ImpendingHardRemovalConflictDetection catch-up hooks disabled in Testing (their non-ORM tables are dropped by the test schema compile). Semantic change (documented in NodeServiceWorkspacesTest): nodes disabled in a user workspace are now correctly excluded from that workspace's results and, after publishing, from live - the Neos 8 suite documented the old SQL-based behavior as a quirk. Result: 23 tests, 255 assertions, all green (plus 19 unit tests).
Configure -> auto-generation via real API -> save-all -> persistence verified (saved asset drops off the unset-only listing). Exercises findAssets, updateAssets and the Flow 9 AssetService iteration fix end-to-end. Requires a publicly reachable PLAYWRIGHT_BASE_URL (ddev share) so the NEOSidekick API can fetch the images.
…n notes - New ContentRepositoryProvider (singleton) resolves the content repository from the new NEOSidekick.AiAssistant.contentRepositoryId setting (default 'default'); all 14 inline registry lookups now go through it, so a future multi-CR implementation (e.g. via SiteDetectionResult) is a one-place change. - All remaining 'TODO 9.0 migration' comments resolved: the multiple-CR ones by the provider, the '(manual)' ones reworded to 'NOTE (Neos 9 migration decision)' - they document deliberate, permanent semantic differences (result ordering, dimension fallback collapsing, non-transactional patch application, origin-DSP property writes), not open work. Verified: 19 unit + 23 functional + 4 Playwright E2E tests green.
Generality review results: - Functional tests no longer assume the Neos.Demo dimension configuration (de/en_US constants, /de/ URL literals): the test base now derives the two test languages and their URI segments from the running content repository and site configuration (primaryLanguage() prefers a non-site-default value so test URLs keep their segment prefix). Dimension-variant tests skip themselves on distributions with fewer than two top-level language values; the suffix helper reads the site's uriPathSuffix. - E2E specs renamed (neos9.editorsAndChat / neos9.bulkAltTextModule): they run on any Neos 9 site with the plugin's default configuration, not only Neos.Demo. Docblocks now state the exact requirements (API key, English backend UI, public base URL for the bulk module) and warn that they WRITE to the instance. The bulk module persistence assertion is now identity-based (first-listed asset must change) instead of page-count-based, which broke on repeated runs. - README: Neos compatibility matrix, the contentRepositoryId setting, and the LostInTranslation dimension-options relocation documented; TESTING.md documents the MariaDB/MySQL test-database requirement (the content graph does not support the SQLite Testing default) and the dimension adaptivity; Tests/E2E/README.md documents the Neos 9 specs. Production code was audited for project-specific assumptions: none found (the 'language' occurrences are DTO field names / API payload keys; dimension and CR access are driven by the languageDimensionName / contentRepositoryId settings). The Neos.Demo:* node type overrides under Configuration/Testing pre-date this migration and only apply in the Testing context. Verified: 19 unit + 23 functional + 4 E2E green after the refactor.
The Neos 9 content graph requires MariaDB/MySQL, so the functional tests can no longer run on the base distribution's SQLite Testing default. Following the setup of neos/neos-development-collection's build.yml: - matrix targets Neos 9.0 + 9.1 only (this branch is a clean Neos 9 fork, main keeps the 8.x pipeline); temporary push trigger for the feature branch so the pipeline itself can be tested before merge - mariadb:10.11 service container (health-checked, db flow_functional_testing) - new step overwrites the base distribution's Testing Settings.yaml with the service connection (mirror of core's configure-neos composite action); the plugin's own Configuration/Testing contributes the rest - pdo_sqlite extension dropped Also fixes 5 auto-fixable PSR-12 violations introduced by the migration (multi-line control structures) found while pre-verifying the phpcs step locally; phpstan (CI configuration) verified clean, functional suite re-run green after the fixes.
UserService::getPersonalWorkspaceName() still exists in Neos 9 but throws (neos/neos-development-collection#5418) - it survived the migration unnoticed because Flow proxy compilation and the test suites never call it: only the JWT-authenticated agent API endpoints do, at runtime. All five callers (SearchNodes, DocumentNodeList, GetPreview, ApplyPatches, NodeTreeSchema) returned 500 for every chat agent action that fetches page content. New PersonalWorkspaceService resolves the current user's personal workspace through Neos\Neos\Domain\Service\WorkspaceService metadata and preserves the old null contract (no user / no workspace assigned -> null), so the requested-workspace fallbacks in the controllers behave as before. The legacy Neos\Neos\Service\UserService injections became unused and were removed. Verified: 19 unit + 23 functional green, phpcs/phpstan clean; unauthenticated endpoint returns 401 (auth layer unaffected).
…l generate waits - @neos-project/neos-ui-extensibility + react-ui-components ^8.3 -> ^9.1.6, @fortawesome/free-* ^5 -> ^6.5.2 (9.x peer requirement); Plugin.js/.css and BackendModule.js rebuilt from the updated stack (react stays 16-typed per the 9.1.6 peer range - the host UI provides React at runtime via the extensibility alias map). - .npmrc pins the @FortAwesome scope to the public registry: the plugin uses the free icon packages and must not resolve them through a Pro registry configured in a developer's user-level npm config (matches the historical yarn.lock resolution). Robustness: an image alt-text generate button was observed stuck in an endless loading state (not deterministically reproducible; generation itself verified working repeatedly). Every await in the generate chain that could pend forever without an error is now bounded and reports a flash message instead: - ContentService.getGuestFrameContentDocument(): the unbounded 100ms poll loops now fail after 15s (previously spun forever on an unreachable or never-finishing guest frame, e.g. for documentContent-based generations) - ApiService.fetch(): 120s AbortSignal timeout (stalled tunnel connections) - ContentService.getImageMetadata(): 30s race around the host UI's loadImageMetadata endpoint Verified on the rebuilt bundles: all 4 Playwright E2E specs green, alt-text generation on Neos.Demo produces and commits a description.
…ge URIs
The important-pages filter (focus keyword, SEO title & meta description, SEO
image alt-text backend modules) sent one '<host>/<uriSegment>' URL per language
to the NEOSidekick API. In Neos 9 the site-default language's homepage has an
EMPTY uri path ('/en' 404s while '/' works), so the API failed to crawl the
first URL and reported the whole site as not publicly accessible.
The entry URL per language is now the router-generated homepage URI of the
site node in that language's dimension space point (NodeUriBuilder, live
workspace); languages without a resolvable homepage variant are skipped. This
also removes the manual uriSegment-mapping extraction - the router is the
single source of truth for any dimension resolver configuration.
Verified on the demo instance through a public tunnel with the real API: all
three important-pages modules now list pages (previously: 'website must be
publicly accessible'). Functional suite green (23/255) including the strict
host-list mock, unit 19/19, phpcs/phpstan clean, 4 E2E specs green.
… remain The spec's documented precondition (at least one image asset without a title) consumes itself: every run persists titles. Repeated runs on the same instance now skip with an explanatory message instead of failing.
…on save Symptom (SEO title & meta description module): important-pages and custom filter modes appeared inconsistent — custom listed mostly English (US) while important-pages listed English (UK) and German rows. The enumeration difference is correct and matches Neos 8: the custom filter lists one row per EXISTING origin variant (the demo has 46 en_US but only 2 de / 2 en_UK variants), while important-pages lists one row per public URL — including fallback URLs like /uk/... serving en_US content, which the crawler legitimately reports as distinct pages. Underneath sat three real dimension-semantics bugs for such fallback rows (address dimension != origin dimension), all fixed to use the SERVED dimension like the old CR context dimensions: - FindDocumentNodeDataFactory labeled fallback rows with their ORIGIN language (every /uk/... row showed 'English (US)'). - The language dimension filter matched the origin, so filtering by en_UK would have dropped all fallback UK rows. - WORST: updatePropertiesOnNodes wrote to the node's origin — saving an SEO title on a /uk/... row would silently overwrite the /en/... value. Now the variant is materialized first (CreateNodeVariant, copy-on-write like Neos 8 contexts) and the properties are written to the addressed dimension. Verified on the demo instance: all 24 important-pages rows label/dimension consistent (10x en_UK, 13x en_US, 1x de); writing focusKeyword to a /uk fallback row created an en_UK origin variant carrying the value while the en_US origin remained untouched (checked in the content graph). Functional 23/255, unit 19/41, phpcs/phpstan clean.
…licit variants
Design decision (supersedes the copy-on-write behavior introduced in 9d5156a,
and deliberately diverges from Neos 8, which materialized variants implicitly
via context adoption on setProperty):
Fallback pages (a specialization serving its generalization's content, e.g.
en_UK falling back to en_US) are not editable rows. Materializing a variant as
a save side effect copies the ENTIRE property set and permanently detaches the
page from its fallback chain - future origin edits no longer propagate. That
decision belongs to an editor in the Neos UI ('create variant'), never to a
bulk tool or an agent patch.
- NodeService::findImportantPages(): fallback URLs are re-addressed to their
ORIGIN dimension and deduped with the origin's own URL row. The underlying
page stays in the list once, correctly labeled; editing it improves every
URL it shines through to. Real variants keep their own dimension.
- NodeService::updatePropertiesOnNodes(): writes to fallback addresses are
rejected (1752060000001) with a message pointing to explicit variant
creation. find()/findImportantPages() only emit origin-addressed rows, so
this guards against stale or handcrafted addresses.
- NodePatchService (chat agent updateNode): same rejection as PatchFailed.
- Both backend module modes now consistently enumerate editable origin
variants (previously important-pages listed each page once per public URL).
New NodeServiceFallbackDimensionsTest (4 tests) covers: fallback URL re-
addressing, real variants keeping their dimension, fallback write rejection,
variant writes leaving the generalization untouched. Skips on distributions
without a language specialization pair. Test base: createPageWithImageNodes()
accepts an optional language.
Verified live: important-pages went from 24 rows (10 fallback duplicates) to
16 origin rows (13 en_US, 1 de, 2 real en_UK variants). Functional 27/282,
unit 19/41, phpcs/phpstan clean.
…dles) Post-rebase reconciliation after rebasing onto main (d71aa5e + the Neos 8 fallback backport 7bf846b): - Classes/Service/DimensionFallbackDetector.php (from the Neos 8 backport) removed: it is written against the old CR API (NodeInterface/getNodeData); on Neos 9 the fallback concept is implemented inline via origin vs. subgraph dimension space point comparison (NodeService, NodePatchService) and covered by NodeServiceFallbackDimensionsTest. - Plugin.js/.css rebuilt from the merged sources: the rebase brought main's frontend changes (AiModal link styling + loading indicator, CKEditor css, ContentCanvasService page-updated handling) which are now compiled into the Neos 9 bundle; before the rebuild the published bundle mixed both sides. Main's other incoming features verified present and Neos-9-compatible: BaseUriProvider-based domain resolution behind reverse proxies (NEOSidekickInternalHelper), per-domain website configuration via siteDomain (ConfigurationController + Root.fusion), and the new ConfigurationControllerSiteDomainTest (pure mock unit test, runs unchanged). Verified: 21 unit / 27 functional / phpcs / phpstan green, backend healthy.
Both 9.1 matrix jobs started failing in the 'Install Neos Project and other dependencies' step on 27.07. (run 30249483831) while the ~9.0.0 jobs kept passing - immediately after the 9.1.7/9.1.8 releases (15.07.). The composer sequence with ~9.1.0 (resolving 9.1.8) is not reproducible as broken locally, so this pins the 9.1 entry to 9.1.6, the release this migration was developed against and the last one verified green in CI (run 29006708375). Revisit to unpin once the newer patch releases are investigated on a real runner.
The neos9.editorsAndChat and neos9.bulkAltTextModule specs were written to verify the migration against the local Neos.Demo instance. They need too much scaffolding to run anywhere else (API key, publicly reachable instance, untitled assets, English backend) and write to the instance they run against, so they do not belong in the repository. The pre-existing E2E suite for the NEOSidekickTestWebsite stays untouched.
gradinarufelix
changed the title
e522e3b to
76be352
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Classes/Service/DocumentNodeListExtractor.php (1)
150-163: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winUse a one-shot descendant query for document extraction.
traverseDocuments()issuesfindChildNodes()for every visited node, so the defaultdepth: -1can traverse the whole site subtree. This also counts all children incurrentDepth, so non-document wrapping nodes can remove reachable documents from a boundeddepthrequest and change thedepthfield. CallfindDescendantNodes()withFindDescendantNodesFilter::create(nodeTypes: $nodeTypeFilter), then compute document-specific depth if the consumer needs it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Classes/Service/DocumentNodeListExtractor.php` around lines 150 - 163, Replace the recursive child traversal in traverseDocuments() with a single findDescendantNodes() query using FindDescendantNodesFilter::create(nodeTypes: $nodeTypeFilter). Remove the per-node recursion and ensure returned documents are extracted directly, computing depth based on document-relevant ancestry rather than all wrapping nodes if the depth field is required.
🧹 Nitpick comments (5)
Classes/Service/DocumentNodeListExtractor.php (1)
115-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
$contentRepositoryparameter.
resolveSiteNode()does not use$contentRepository. PHPMD flags it. Drop the parameter and the argument at line 89.♻️ Proposed change
- $siteNode = $this->resolveSiteNode($contentRepository, $subgraph, $siteNodeName); + $siteNode = $this->resolveSiteNode($subgraph, $siteNodeName);- private function resolveSiteNode(ContentRepository $contentRepository, ContentSubgraphInterface $subgraph, ?string $siteNodeName): ?Node + private function resolveSiteNode(ContentSubgraphInterface $subgraph, ?string $siteNodeName): ?Node🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Classes/Service/DocumentNodeListExtractor.php` at line 115, Remove the unused $contentRepository parameter from DocumentNodeListExtractor::resolveSiteNode(), and update its call site around line 89 to stop passing that argument while preserving the remaining parameters and behavior.Source: Linters/SAST tools
Tests/Functional/FunctionalTestCase.php (2)
364-380: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFixture helpers do not derive the target workspace from the node they act on. Both helpers pick the workspace independently of the passed
Node, so a node read from a user-workspace subgraph can be written tolivewithout an error.
Tests/Functional/FunctionalTestCase.php#L364-L380: replace the hardcodedWorkspaceName::forLive()andsubgraph('live', $language)with$parentNode->workspaceName.Tests/Functional/FunctionalTestCase.php#L399-L407: default$workspaceto$node->workspaceNameinstead of'live'.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/Functional/FunctionalTestCase.php` around lines 364 - 380, The fixture helpers must use the workspace associated with their input node. In Tests/Functional/FunctionalTestCase.php lines 364-380, update createPageWithImageNodes to use $parentNode->workspaceName for both CreateNodeAggregateWithNode and subgraph instead of the hardcoded live workspace. In lines 399-407, default $workspace to $node->workspaceName rather than 'live'.
255-261: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Neos.Neos.sites.*reads only the default site configuration.Flow's
ConfigurationManagerdoes not expand*as a wildcard. It resolves the literal*key, which Neos 9 uses for default site settings. Per-site overrides underNeos.Neos.sites.exampleare therefore ignored, andlanguageUriSegment()plusgetUriPathSuffix()can disagree with the site the test drives.Add a short comment that states the literal-key intent, or resolve the concrete site node name when it is known.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/Functional/FunctionalTestCase.php` around lines 255 - 261, Clarify the literal-key behavior in siteConfigurationPath() by adding a concise comment explaining that Neos.Neos.sites.* intentionally reads the default site settings, or change the lookup to use the concrete site node name when the driven site is known. Ensure languageUriSegment() and getUriPathSuffix() resolve configuration for the same site.Classes/Controller/SearchNodesApiController.php (1)
40-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the duplicated workspace resolution.
resolveWorkspace()and thepersonalWorkspaceServiceproperty are now identical inClasses/Controller/SearchNodesApiController.php,Classes/Controller/DocumentNodeListApiController.php,Classes/Controller/GetPreviewApiController.php, andClasses/Controller/NodeTreeSchemaApiController.php. Move the logic intoPersonalWorkspaceService(for exampleresolveWorkspace(string $requestedWorkspace): string) or a shared trait. This keeps the four endpoints consistent when the fallback rule changes.Also applies to: 141-149
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Classes/Controller/SearchNodesApiController.php` around lines 40 - 41, Extract the duplicated workspace-resolution logic from resolveWorkspace() in SearchNodesApiController and the corresponding methods in DocumentNodeListApiController, GetPreviewApiController, and NodeTreeSchemaApiController into PersonalWorkspaceService, exposing a shared resolveWorkspace(string $requestedWorkspace): string operation. Update all four controllers to delegate to the service and remove their duplicate resolution code while preserving the existing fallback behavior.Tests/Functional/Service/NodeServiceFallbackDimensionsTest.php (1)
148-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the fixture nodes exist before you read
aggregateId.
getNodeByPath()can returnnull. Lines 153 and 182 then dereferencenulland the test fails with a confusing "property on null" error instead of a clear precondition failure. AddassertNotNull()after each lookup, asaddressForPath()inTests/Functional/FunctionalTestCase.phpalready does.Also applies to: 177-183
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/Functional/Service/NodeServiceFallbackDimensionsTest.php` around lines 148 - 154, Update both node lookups in the fallback-dimensions test, including the lookup used before the second address construction, to call assertNotNull() immediately after getNodeByPath(). Keep using each validated node’s aggregateId so missing fixture nodes produce a clear precondition failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Classes/Service/DocumentNodeListExtractor.php`:
- Around line 86-87: Update the subgraph visibility configuration in
DocumentNodeListExtractor so the isHidden value derived from
NeosSubtreeTag::disabled() remains meaningful: use
NodeVisibility::excludeRemoved() instead of excluding disabled nodes. Preserve
the existing isHidden response mapping and continue excluding removed nodes.
- Around line 175-177: Update the path filtering around
SearchNodesApiController::searchAction() so legacy pathStartingPoint values such
as /sites/example continue matching the new /<Neos.Neos:Sites>/site/... document
paths. Normalize the requested prefix to the new absolute format before
str_starts_with(), or consistently convert the serialized root segment to /sites
in DocumentNodeListExtractor::tryRetrieveNodePath() output.
- Around line 215-220: Update the dimension-coordinate handling in the visible
extraction method to validate the point created by
DimensionSpacePoint::fromArray() against the variation graph before returning
it. Reject invalid client-supplied coordinates with an accurate error before
extract() queries content, while preserving the existing root-generalization and
createWithoutDimensions fallbacks for empty coordinates.
In `@Classes/Service/NodeService.php`:
- Around line 200-202: Qualify all three InvalidArgumentException throws in
NodeService methods find(), updatePropertiesOnNode(), and the fallback-address
write path with the global exception class, or import it explicitly at the top
of the file. Ensure unknown workspaces, missing nodes, and fallback-address
writes throw the intended global InvalidArgumentException.
In `@Classes/Service/NodeWithImageService.php`:
- Around line 99-104: Update the language filter block in the relevant
NodeWithImageService method to check that languageDimensionName is configured
before constructing ContentDimensionId or looking up the coordinate, matching
the existing guard in NodeService::nodeMatchesLanguageDimensionFilter().
Preserve the current filtering behavior when the dimension name is set.
In `@Tests/Functional/FunctionalTestCase.php`:
- Around line 267-274: Update dimensionSpacePoint() to handle a non-null
language when languageDimensionId() returns null: resolve the dimension ID
before accessing value and skip clearly instead of dereferencing null. Preserve
createWithoutDimensions() when no language is available and fromArray() when the
language dimension exists.
In `@Tests/Functional/Service/NodeServiceMultiSiteTest.php`:
- Around line 55-58: Extend the wrong-site assertions in the node result checks
around $foundNodes to cover the secondary language dimension by generating
equivalent addresses with $this->secondaryLanguage() for every excluded
/sites/example2 node, including the assertions referenced in the additional
location. Preserve the existing default-language assertions.
---
Outside diff comments:
In `@Classes/Service/DocumentNodeListExtractor.php`:
- Around line 150-163: Replace the recursive child traversal in
traverseDocuments() with a single findDescendantNodes() query using
FindDescendantNodesFilter::create(nodeTypes: $nodeTypeFilter). Remove the
per-node recursion and ensure returned documents are extracted directly,
computing depth based on document-relevant ancestry rather than all wrapping
nodes if the depth field is required.
---
Nitpick comments:
In `@Classes/Controller/SearchNodesApiController.php`:
- Around line 40-41: Extract the duplicated workspace-resolution logic from
resolveWorkspace() in SearchNodesApiController and the corresponding methods in
DocumentNodeListApiController, GetPreviewApiController, and
NodeTreeSchemaApiController into PersonalWorkspaceService, exposing a shared
resolveWorkspace(string $requestedWorkspace): string operation. Update all four
controllers to delegate to the service and remove their duplicate resolution
code while preserving the existing fallback behavior.
In `@Classes/Service/DocumentNodeListExtractor.php`:
- Line 115: Remove the unused $contentRepository parameter from
DocumentNodeListExtractor::resolveSiteNode(), and update its call site around
line 89 to stop passing that argument while preserving the remaining parameters
and behavior.
In `@Tests/Functional/FunctionalTestCase.php`:
- Around line 364-380: The fixture helpers must use the workspace associated
with their input node. In Tests/Functional/FunctionalTestCase.php lines 364-380,
update createPageWithImageNodes to use $parentNode->workspaceName for both
CreateNodeAggregateWithNode and subgraph instead of the hardcoded live
workspace. In lines 399-407, default $workspace to $node->workspaceName rather
than 'live'.
- Around line 255-261: Clarify the literal-key behavior in
siteConfigurationPath() by adding a concise comment explaining that
Neos.Neos.sites.* intentionally reads the default site settings, or change the
lookup to use the concrete site node name when the driven site is known. Ensure
languageUriSegment() and getUriPathSuffix() resolve configuration for the same
site.
In `@Tests/Functional/Service/NodeServiceFallbackDimensionsTest.php`:
- Around line 148-154: Update both node lookups in the fallback-dimensions test,
including the lookup used before the second address construction, to call
assertNotNull() immediately after getNodeByPath(). Keep using each validated
node’s aggregateId so missing fixture nodes produce a clear precondition
failure.
🪄 Autofix
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 Plus
Run ID: 090ba227-abcb-4157-9702-cbf9d2cea082
⛔ Files ignored due to path filters (2)
Resources/Private/NeosUserInterface/yarn.lockis excluded by!**/yarn.lock,!**/*.lockResources/Public/NeosUserInterface/Plugin.js.mapis excluded by!**/*.map
📒 Files selected for processing (49)
.github/workflows/test.ymlClasses/Controller/ApplyPatchesApiController.phpClasses/Controller/BackendModule/AbstractFusionViewController.phpClasses/Controller/BackendServiceController.phpClasses/Controller/DocumentNodeListApiController.phpClasses/Controller/GetPreviewApiController.phpClasses/Controller/NodeTreeSchemaApiController.phpClasses/Controller/PreviewRenderController.phpClasses/Controller/SearchNodesApiController.phpClasses/EelHelper/NEOSidekickHelper.phpClasses/EelHelper/NEOSidekickInternalHelper.phpClasses/Factory/FindDocumentNodeDataFactory.phpClasses/Factory/FindImageDataFactory.phpClasses/Service/AbstractNodeService.phpClasses/Service/ContentRepositoryProvider.phpClasses/Service/DocumentNodeListExtractor.phpClasses/Service/NodeFindingService.phpClasses/Service/NodePatchService.phpClasses/Service/NodeService.phpClasses/Service/NodeTreeExtractor.phpClasses/Service/NodeTypeSchemaExtractor.phpClasses/Service/NodeTypeService.phpClasses/Service/NodeVisibility.phpClasses/Service/NodeWithImageService.phpClasses/Service/PatchValidator.phpClasses/Service/PersonalWorkspaceService.phpClasses/Service/PropertyNormalizer.phpClasses/Service/SearchNodesExtractor.phpClasses/Service/SiteService.phpClasses/Service/Traits/PropertyExtractionTrait.phpConfiguration/Settings.Internal.yamlConfiguration/Settings.yamlConfiguration/Testing/NodeTypes.HomePage.yamlConfiguration/Testing/Settings.yamlREADME.mdResources/Private/BackendModule/BackendModule.fusionResources/Private/BackendModule/Root.fusionResources/Private/NeosUserInterface/package.jsonResources/Public/NeosUserInterface/Plugin.jsTESTING.mdTests/E2E/README.mdTests/Functional/FunctionalTestCase.phpTests/Functional/Service/NodeServiceFallbackDimensionsTest.phpTests/Functional/Service/NodeServiceImportantPagesMultiDomainTest.phpTests/Functional/Service/NodeServiceMultiSiteTest.phpTests/Functional/Service/NodeServiceWithImportantPagesFilterAndMultipleDimensionsAndOneSiteTest.phpTests/Functional/Service/NodeServiceWithMultipleDimensionsAndOneSiteTest.phpTests/Functional/Service/NodeServiceWorkspacesTest.phpcomposer.json
💤 Files with no reviewable changes (2)
- Classes/Controller/BackendServiceController.php
- Classes/Service/SiteService.php
🚧 Files skipped from review as they are similar to previous changes (30)
- Configuration/Settings.yaml
- Classes/Service/ContentRepositoryProvider.php
- Configuration/Testing/Settings.yaml
- Resources/Private/BackendModule/Root.fusion
- README.md
- composer.json
- Configuration/Settings.Internal.yaml
- Tests/Functional/Service/NodeServiceImportantPagesMultiDomainTest.php
- Classes/Service/PropertyNormalizer.php
- Classes/Factory/FindImageDataFactory.php
- Classes/Controller/BackendModule/AbstractFusionViewController.php
- Classes/EelHelper/NEOSidekickHelper.php
- Tests/Functional/Service/NodeServiceWorkspacesTest.php
- Resources/Private/BackendModule/BackendModule.fusion
- Classes/Service/AbstractNodeService.php
- Classes/Service/NodeTypeSchemaExtractor.php
- Tests/Functional/Service/NodeServiceWithMultipleDimensionsAndOneSiteTest.php
- Classes/EelHelper/NEOSidekickInternalHelper.php
- Classes/Factory/FindDocumentNodeDataFactory.php
- Tests/Functional/Service/NodeServiceWithImportantPagesFilterAndMultipleDimensionsAndOneSiteTest.php
- Classes/Service/PatchValidator.php
- Classes/Service/NodeTypeService.php
- Classes/Service/SearchNodesExtractor.php
- Classes/Service/NodeTreeExtractor.php
- Classes/Controller/PreviewRenderController.php
- Classes/Service/Traits/PropertyExtractionTrait.php
- Classes/Service/NodePatchService.php
- Classes/Service/NodeVisibility.php
- TESTING.md
- Configuration/Testing/NodeTypes.HomePage.yaml
| $subgraph = $contentRepository->getContentGraph($workspaceObject->workspaceName) | ||
| ->getSubgraph($dimensionSpacePoint, NodeVisibility::excludeDisabledAndRemoved()); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
isHidden in the response is now always false.
The subgraph uses NodeVisibility::excludeDisabledAndRemoved(), so disabled nodes are never returned. Line 183 still derives isHidden from NeosSubtreeTag::disabled(). That field can therefore never be true, and API consumers lose the previous signal.
Pick one behavior:
- Keep frontend-like reading and drop
isHidden, or document it as alwaysfalse. - Or read with
NodeVisibility::excludeRemoved()so disabled nodes stay visible andisHiddenstays meaningful.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Classes/Service/DocumentNodeListExtractor.php` around lines 86 - 87, Update
the subgraph visibility configuration in DocumentNodeListExtractor so the
isHidden value derived from NeosSubtreeTag::disabled() remains meaningful: use
NodeVisibility::excludeRemoved() instead of excluding disabled nodes. Preserve
the existing isHidden response mapping and continue excluding removed nodes.
| // NOTE (Neos 9 migration decision): node paths now use the absolute path format | ||
| // "/<Neos.Neos:Sites>/site/..." instead of the legacy "/sites/site/..." format. | ||
| 'path' => $this->tryRetrieveNodePath($subgraph, $node) ?? '', |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The new path format silently breaks pathStartingPoint filtering.
SearchNodesApiController::searchAction() filters this result with str_starts_with($document['path'], $pathStartingPoint). The path now starts with /<Neos.Neos:Sites>/. A caller that passes a legacy prefix such as /sites/example receives an empty document list and no error.
Normalize the prefix in the filter, or map the serialized root segment to /sites before returning path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Classes/Service/DocumentNodeListExtractor.php` around lines 175 - 177, Update
the path filtering around SearchNodesApiController::searchAction() so legacy
pathStartingPoint values such as /sites/example continue matching the new
/<Neos.Neos:Sites>/site/... document paths. Normalize the requested prefix to
the new absolute format before str_starts_with(), or consistently convert the
serialized root segment to /sites in
DocumentNodeListExtractor::tryRetrieveNodePath() output.
| if ($coordinates !== []) { | ||
| return DimensionSpacePoint::fromArray($coordinates); | ||
| } | ||
| $rootGeneralizations = $contentRepository->getVariationGraph()->getRootGeneralizations(); | ||
| return $rootGeneralizations !== [] ? reset($rootGeneralizations) : DimensionSpacePoint::createWithoutDimensions(); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the variation graph API and DimensionSpacePoint factory in the installed CR.
fd -t f 'InterDimensionalVariationGraph.php' | xargs -r -I{} rg -n -A12 'function getRootGeneralizations' {}
fd -t f 'DimensionSpacePoint.php' -p 'ContentRepository' | xargs -r -I{} rg -n -A15 'public static function fromArray' {}Repository: NEOSidekick/NEOSidekick.AiAssistant
Length of output: 278
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Repository top-level files/dirs:\n'
git ls-files | sed 's#/.*##' | sort -u | head -100
printf '\nFind target file:\n'
fd -t f 'DocumentNodeListExtractor.php' . || true
printf '\nFind Neos variation graph / dimension point candidates in tracked files:\n'
git ls-files | rg -i 'InterDimensionalVariationGraph|DimensionSpacePoint|ContentRepositoryVariant|DocumentNodeListExtractor' || true
printf '\nInspect target file around resolveDimensionSpacePoint:\n'
file="$(fd -t f 'DocumentNodeListExtractor.php' . | head -n 1)"
if [ -n "${file:-}" ]; then
wc -l "$file"
sed -n '1,260p' "$file" | cat -n
fiRepository: NEOSidekick/NEOSidekick.AiAssistant
Length of output: 11516
🌐 Web query:
Neos 9 ContentRepository getVariationGraph getRootGeneralizations DimensionSpacePoint signature
💡 Result:
In Neos 9.0, the InterDimensionalVariationGraph provides methods to interact with the dimension space. The getRootGeneralizations method is used to retrieve the root dimension space points within the variation graph [1]. The signature for getRootGeneralizations in the Neos\ContentRepository\Core\DimensionSpace\InterDimensionalVariationGraph class is: public function getRootGeneralizations: array This method returns an array of DimensionSpacePoint objects representing the root generalizations, indexed by their hash [1]. It is guaranteed to return at least one element, representing the empty dimension space point in a zero-dimensional content repository [1][2]. To access the variation graph from the ContentRepository instance, you use the getVariationGraph() method [3][4]. The workflow typically looks like this: $contentRepository = $contentRepositoryRegistry->get($contentRepositoryId); $variationGraph = $contentRepository->getVariationGraph; $rootGeneralizations = $variationGraph->getRootGeneralizations; The DimensionSpacePoint is a core concept in Neos 9, representing a specific point in the dimension space defined by coordinates (DimensionName => DimensionValue) [5]. It is frequently used in conjunction with the variation graph to determine generalizations, specializations, or to navigate content subgraphs [1][6].
Citations:
- 1: https://neos.github.io/neos/9.0/Neos/ContentRepository/Core/DimensionSpace/InterDimensionalVariationGraph.html
- 2: TASK: Document NodeAggregate neos/neos-development-collection#4832
- 3: https://neos.github.io/neos/9.0/Neos/ContentRepository/Core/ContentRepository.html
- 4: https://github.com/neos/neos-development-collection/blob/9.0/Neos.ContentRepository.Core/Classes/ContentRepository.php
- 5: https://neos.github.io/neos/9.0/Neos/ContentRepository/Core/DimensionSpace.html
- 6: https://github.com/neos/neos-development-collection/blob/9.0/Neos.Neos/Classes/Fusion/DimensionsMenuItemsImplementation.php
🌐 Web query:
Neos 9 DimensionSpacePoint fromArray source code
💡 Result:
In Neos 9, the DimensionSpacePoint class is defined within the Neos.ContentRepository.Core package [1]. The static method fromArray(array $data) is used to create a DimensionSpacePoint instance from an array of coordinate data [1]. While the full implementation detail is managed through the inheritance chain in AbstractDimensionSpacePoint.php [2], the static factory method is provided to ensure proper instantiation of dimension space points based on the provided coordinate mapping [1]. You can view the specific API documentation for this method at the official Neos 9.0 documentation portal [1]. Additionally, usage of this method can be seen in components such as DimensionSpacePointSet, which utilizes it when iterating over point candidates [3], or in frontend routing resolution logic [4].
Citations:
- 1: https://neos.github.io/neos/9.0/Neos/ContentRepository/Core/DimensionSpace/DimensionSpacePoint.html
- 2: https://github.com/neos/neos-development-collection/blob/9.0/Neos.ContentRepository.Core/Classes/DimensionSpace/AbstractDimensionSpacePoint.php
- 3: https://github.com/neos/neos-development-collection/blob/9.0/Neos.ContentRepository.Core/Classes/DimensionSpace/DimensionSpacePointSet.php
- 4: https://github.com/neos/neos-development-collection/blob/9.0/Neos.Neos/Classes/FrontendRouting/DimensionResolution/RequestToDimensionSpacePointContext.php
Validate the client-supplied dimension coordinates before querying the subgraph.
getRootGeneralizations() returns an array of DimensionSpacePoint objects, so the fallback return type is correct. If any dimension array value is not a valid dimension value in the variation graph, DimensionSpacePoint::fromArray() returns an empty point and the request fails with No site found; validate the built point against the variation graph or throw a more accurate error before extract() queries content.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Classes/Service/DocumentNodeListExtractor.php` around lines 215 - 220, Update
the dimension-coordinate handling in the visible extraction method to validate
the point created by DimensionSpacePoint::fromArray() against the variation
graph before returning it. Reject invalid client-supplied coordinates with an
accurate error before extract() queries content, while preserving the existing
root-generalization and createWithoutDimensions fallbacks for empty coordinates.
| if (!$workspace) { | ||
| throw new InvalidArgumentException('The given workspace does not exist in the database. Please reload the page.', 1713440899886); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
InvalidArgumentException is unqualified and unimported — every throw fatals.
This file declares namespace NEOSidekick\AiAssistant\Service and imports no InvalidArgumentException (Lines 7-31). The bare name therefore resolves to NEOSidekick\AiAssistant\Service\InvalidArgumentException, which does not exist. All three throw sites fail with a class-not-found fatal error instead of the intended exception:
- Line 201: an unknown workspace in
find(). - Line 273: a missing node in
updatePropertiesOnNode(). - Line 282: a write to a fallback address.
Tests/Functional/Service/NodeServiceFallbackDimensionsTest.php imports the global InvalidArgumentException on Line 5 and asserts it on Line 159, so updateRejectsWritesToFallbackAddresses cannot pass with the current code.
🐛 Proposed fix
use Neos\ContentRepository\Core\Dimension\ContentDimensionId;
+use InvalidArgumentException;Also applies to: 272-288
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Classes/Service/NodeService.php` around lines 200 - 202, Qualify all three
InvalidArgumentException throws in NodeService methods find(),
updatePropertiesOnNode(), and the fallback-address write path with the global
exception class, or import it explicitly at the top of the file. Ensure unknown
workspaces, missing nodes, and fallback-address writes throw the intended global
InvalidArgumentException.
| if (!empty($languageDimensionFilter)) { | ||
| $languageCoordinate = $documentNodeAddress->dimensionSpacePoint->getCoordinate(new ContentDimensionId($this->languageDimensionName)); | ||
| if ($languageCoordinate !== null && !in_array($languageCoordinate, $languageDimensionFilter, true)) { | ||
| continue; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard the language dimension lookup when languageDimensionName is not configured.
$this->languageDimensionName comes from @Flow\InjectConfiguration(path="languageDimensionName"). If the setting is absent, the property stays null and new ContentDimensionId(null) on Line 100 raises a TypeError. NodeService::nodeMatchesLanguageDimensionFilter() guards this case with isset($this->languageDimensionName). Apply the same guard here so both services behave the same for distributions without a language dimension.
🛡️ Proposed guard
- if (!empty($languageDimensionFilter)) {
+ if (!empty($languageDimensionFilter) && !empty($this->languageDimensionName)) {
$languageCoordinate = $documentNodeAddress->dimensionSpacePoint->getCoordinate(new ContentDimensionId($this->languageDimensionName));📝 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.
| if (!empty($languageDimensionFilter)) { | |
| $languageCoordinate = $documentNodeAddress->dimensionSpacePoint->getCoordinate(new ContentDimensionId($this->languageDimensionName)); | |
| if ($languageCoordinate !== null && !in_array($languageCoordinate, $languageDimensionFilter, true)) { | |
| continue; | |
| } | |
| } | |
| if (!empty($languageDimensionFilter) && !empty($this->languageDimensionName)) { | |
| $languageCoordinate = $documentNodeAddress->dimensionSpacePoint->getCoordinate(new ContentDimensionId($this->languageDimensionName)); | |
| if ($languageCoordinate !== null && !in_array($languageCoordinate, $languageDimensionFilter, true)) { | |
| continue; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Classes/Service/NodeWithImageService.php` around lines 99 - 104, Update the
language filter block in the relevant NodeWithImageService method to check that
languageDimensionName is configured before constructing ContentDimensionId or
looking up the coordinate, matching the existing guard in
NodeService::nodeMatchesLanguageDimensionFilter(). Preserve the current
filtering behavior when the dimension name is set.
| protected function dimensionSpacePoint(?string $language = null): DimensionSpacePoint | ||
| { | ||
| $nodeTemplate = new NodeTemplate(); | ||
| $nodeTemplate->setNodeType($this->nodeTypeManager->getNodeType('NEOSidekick.AiAssistant.Testing:Image')); | ||
| $nodeTemplate->setProperty('image', $this->importImage($imageFixtureFilename)); | ||
| return $nodeTemplate; | ||
| $language = $language ?? $this->primaryLanguage(); | ||
|
|
||
| return $language === null | ||
| ? DimensionSpacePoint::createWithoutDimensions() | ||
| : DimensionSpacePoint::fromArray([$this->languageDimensionId()->value => $language]); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Null dereference when a language is passed and no language dimension exists.
languageDimensionId() returns null on distributions without the configured language dimension. Line 273 then calls ->value on null and the test fails with a fatal error instead of a clear skip. The check on line 271 only covers $language === null.
🐛 Proposed fix
protected function dimensionSpacePoint(?string $language = null): DimensionSpacePoint
{
$language = $language ?? $this->primaryLanguage();
+ $dimensionId = $this->languageDimensionId();
- return $language === null
+ return ($language === null || $dimensionId === null)
? DimensionSpacePoint::createWithoutDimensions()
- : DimensionSpacePoint::fromArray([$this->languageDimensionId()->value => $language]);
+ : DimensionSpacePoint::fromArray([$dimensionId->value => $language]);
}📝 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.
| protected function dimensionSpacePoint(?string $language = null): DimensionSpacePoint | |
| { | |
| $nodeTemplate = new NodeTemplate(); | |
| $nodeTemplate->setNodeType($this->nodeTypeManager->getNodeType('NEOSidekick.AiAssistant.Testing:Image')); | |
| $nodeTemplate->setProperty('image', $this->importImage($imageFixtureFilename)); | |
| return $nodeTemplate; | |
| $language = $language ?? $this->primaryLanguage(); | |
| return $language === null | |
| ? DimensionSpacePoint::createWithoutDimensions() | |
| : DimensionSpacePoint::fromArray([$this->languageDimensionId()->value => $language]); | |
| } | |
| protected function dimensionSpacePoint(?string $language = null): DimensionSpacePoint | |
| { | |
| $language = $language ?? $this->primaryLanguage(); | |
| $dimensionId = $this->languageDimensionId(); | |
| return ($language === null || $dimensionId === null) | |
| ? DimensionSpacePoint::createWithoutDimensions() | |
| : DimensionSpacePoint::fromArray([$dimensionId->value => $language]); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Tests/Functional/FunctionalTestCase.php` around lines 267 - 274, Update
dimensionSpacePoint() to handle a non-null language when languageDimensionId()
returns null: resolve the dimension ID before accessing value and skip clearly
instead of dereferencing null. Preserve createWithoutDimensions() when no
language is available and fromArray() when the language dimension exists.
| $this->assertArrayNotHasKey($this->addressForPath('/sites/example2', $this->currentUserWorkspace), $foundNodes); | ||
| $this->assertArrayNotHasKey($this->addressForPath('/sites/example2/node-two-wan-kenodi', $this->currentUserWorkspace), $foundNodes); | ||
| $this->assertArrayNotHasKey($this->addressForPath('/sites/example2/node-two-wan-kenodi/lady-eleonode-rootford-2', $this->currentUserWorkspace), $foundNodes); | ||
| $this->assertArrayNotHasKey($this->addressForPath('/sites/example2/node-two-mc-nodeface', $this->currentUserWorkspace), $foundNodes); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Check wrong-site nodes in every language dimension.
The setup creates secondary-language variants for both sites. These assertions only reject addresses from the default dimension.
A wrong-site secondary-language node can remain in $foundNodes without failing the tests. Add equivalent assertions with $this->secondaryLanguage(), or verify the site of every returned address.
Also applies to: 78-81
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Tests/Functional/Service/NodeServiceMultiSiteTest.php` around lines 55 - 58,
Extend the wrong-site assertions in the node result checks around $foundNodes to
cover the secondary language dimension by generating equivalent addresses with
$this->secondaryLanguage() for every excluded /sites/example2 node, including
the assertions referenced in the additional location. Preserve the existing
default-language assertions.
main is renamed to 4.x (Neos 9 line); 3.x carries the Neos 8.3/8.4 line. The temporary feature-branch trigger is no longer needed.
1 participant
What this PR does
Migrates NEOSidekick.AiAssistant to Neos 9.0/9.1 and its event-sourced Content
Repository. This branch is a clean fork of main and becomes the 4.x release line
(main was renamed to 4.x on merge); the 3.x branch continues to serve Neos 8.3/8.4. All decisions made during the migration
are additionally documented in a decision log (available on request — it was kept in
the demo project used for verification).
Supported: Neos 9.0 / 9.1, PHP 8.3 / 8.4 · Dropped: Neos 8.x support on this line
Review guide
The history is ordered so mechanical and manual changes stay separable:
Require Neos 9 packages…sitegeist/fusionlinkprototypes(no Neos 9 release; coreNeos.Fusion:ActionUrireplaces it)Apply neos/rector…Manual Neos 9 migration…Key design decisions
NodeAddressJSON (workspace +dimensions + aggregate id), round-tripped as an opaque string — the Neos 9
equivalent of the old context paths. Endpoints accept plain aggregate ids too.
ContentRepositoryProvider, configurable viaNEOSidekick.AiAssistant.contentRepositoryId(default
default). Multi-CR support later is a one-place change.important-pages re-addresses fallback URLs to their origin variant; writes to
fallback addresses are rejected instead of materializing a variant as a save side
effect. Variant creation stays an explicit editor decision in the Neos UI.
all patches are validated up front,
dryRunstops after validation.workspace is now correctly excluded from that workspace's results (the 2.x suite
documented the old SQL-based behavior as a known quirk).
Notable fixes found during verification
Neos.Ui.WorkspaceEel helper is no longer in Fusion's default context in Neos 9 —registered by the plugin, backend modules broke without it.
UserService::getPersonalWorkspaceName()is a throwing stub in Neos 9 — all fiveagent API endpoints 500ed; replaced by a
PersonalWorkspaceService(WorkspaceServicemetadata, old null contract preserved).
host/<uriSegment>entry URLs; in Neos 9 the site-defaultlanguage's homepage has an empty URI path, so the NEOSidekick API crawled a 404 and
reported the site as "not publicly accessible". Entry URLs are now router-generated
homepage URIs per language.
(guest-frame polling, API fetch, image metadata) and surfaces a flash message
instead of an endless spinner (backported to main).
Upgrade notes for site integrators (also in README)
contentRepositoryId(defaultdefault).translationStrategy)must move with the dimension configuration into the CR registry settings.
@neos-project/*9.1.6, FontAwesome 6; bundles rebuilt.Testing
~9.0.0) + 9.1 (pinned9.1.6— 9.1.7/9.1.8 broke theinstall step in CI, see commit message; unpin after investigation) × PHP 8.3/8.4,
with a MariaDB service container (the content graph does not support SQLite —
mirrors neos-development-collection's build.yml).
distribution-agnostic (adapts to the host's content dimensions, skips
dimension-variant tests where fewer than two languages exist). Requires a
MariaDB/MySQL test database (documented in TESTING.md).
real NEOSidekick API: inspector editors (focus keyword, SEO title, image alt text),
all six backend modules incl. bulk generate & save, chat sidebar incl. the full
agent authorization flow, and agent tools (SEO metadata, CTA, structural patches).
Known follow-ups
no functional-test coverage with a stubbed JWT identity — this blind spot is where
the
getPersonalWorkspaceName()500 hid.NodeVisibilitywraps the Neos 9 visibility constraints; revisit before Neos 10.react-ui-components9.1.6 peer range; the host UIprovides React at runtime via the extensibility alias map.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation