Conversation
There was a problem hiding this comment.
Pull request overview
This PR changes the “save configuration” flow so saving XML no longer reformats/normalizes content (preventing cursor/attribute shifting), and updates backend + frontend contracts accordingly.
Changes:
- Backend:
updateConfigurationnow writes raw content to disk and returns no XML payload (controller returns an empty 200 response). - Frontend:
saveConfigurationnow expects no response body; editor no longer replaces content with server-returned XML after save. - Config creation paths now add the flow namespace when creating new XML configuration files; tests updated for the new behaviors.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/org/frankframework/flow/configuration/ConfigurationService.java | Stops XML normalization on update; adds namespace insertion when creating new configs (now introduces XML transform exceptions). |
| src/main/java/org/frankframework/flow/configuration/ConfigurationController.java | Update endpoint now returns ResponseEntity<Void>; add-config endpoint now declares XML-related checked exceptions. |
| src/main/java/org/frankframework/flow/filetree/FileTreeService.java | Creating .xml files delegates to configuration creation; throws clause updated (currently contains a compile-breaking duplicate). |
| src/main/java/org/frankframework/flow/filetree/FileTreeController.java | Propagates XML-related checked exceptions from file creation endpoint. |
| src/main/frontend/app/services/configuration-service.ts | saveConfiguration now returns Promise<void> and drops the previous XML response type. |
| src/main/frontend/app/routes/editor/editor.tsx | Save no longer sets editor content from a server-returned XML response. |
| src/main/frontend/app/routes/studio/canvas/flow.tsx | Awaits git diff refresh after saving. |
| src/test/java/org/frankframework/flow/configuration/ConfigurationServiceTest.java | Updates assertions to match raw-write behavior (no newline/normalization). |
| src/test/java/org/frankframework/flow/configuration/ConfigurationControllerTest.java | Updates mocks to reflect boolean/void-style update response. |
| src/test/java/org/frankframework/flow/filetree/FileTreeServiceTest.java | Updates test method signatures to match new checked exceptions. |
| src/main/java/org/frankframework/flow/adapter/AdapterService.java | Removes a stray blank line. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public Project addConfigurationToFolder(String projectName, String configurationName, String folderPath) | ||
| throws IOException, ApiException { | ||
| throws IOException, ApiException, ParserConfigurationException, SAXException, TransformerException { | ||
| Project project = projectService.getProject(projectName); |
There was a problem hiding this comment.
addConfigurationToFolder() now declares TransformerException. Unlike SAXException/ParserConfigurationException, TransformerException is not handled by GlobalExceptionHandler, so this can leak as a default Spring error response that doesn’t match the frontend’s expected error shape. Handle/wrap TransformerException (e.g., into ApiException) to keep API error responses consistent.
src/main/java/org/frankframework/flow/filetree/FileTreeController.java
Outdated
Show resolved
Hide resolved
src/main/java/org/frankframework/flow/filetree/FileTreeService.java
Outdated
Show resolved
Hide resolved
src/main/java/org/frankframework/flow/configuration/ConfigurationController.java
Outdated
Show resolved
Hide resolved
| public ResponseEntity<ProjectDTO> addConfiguration( | ||
| @PathVariable String projectName, @PathVariable String configName) | ||
| throws ProjectNotFoundException, IOException { | ||
| throws ProjectNotFoundException, IOException, ParserConfigurationException, TransformerException, SAXException { |
There was a problem hiding this comment.
This endpoint now declares TransformerException, but GlobalExceptionHandler does not map TransformerException to the standard ErrorResponse shape. If this exception escapes, Spring’s default error JSON will likely break the frontend apiFetch error parsing (it expects httpStatus/messages). Prefer catching/wrapping XML transform errors into ApiException (or adding a dedicated handler) to keep error responses consistent.
| public Project addConfiguration(String projectName, String configurationName) | ||
| throws ProjectNotFoundException, IOException { | ||
| throws ProjectNotFoundException, IOException, TransformerException, ParserConfigurationException, SAXException { | ||
| Project project = projectService.getProject(projectName); |
There was a problem hiding this comment.
addConfiguration() now propagates TransformerException/ParserConfigurationException/SAXException to the controller layer. GlobalExceptionHandler currently handles SAX/ParserConfiguration but not TransformerException, which can lead to inconsistent error payloads for clients. Consider catching TransformerException here and rethrowing an ApiException (or otherwise ensuring it’s handled consistently).
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Stijn Potters <stijn.potters1@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/main/java/org/frankframework/flow/filetree/FileTreeService.java:145
FileTreeService.createFilenow exposes XML-processing checked exceptions (ParserConfigurationException,SAXException,TransformerException) in its public signature even though callers may be creating non-XML files. This leaks implementation details and forces controllers/tests to deal with XML-specific exceptions. Consider catching these exceptions insidecreateFile(or insideConfigurationService.addConfigurationToFolder) and rethrowing anApiException/IOExceptionwith context so the signature can remainthrows IOException, ApiException.
public FileTreeNode createFile(String projectName, String parentPath, String fileName)
throws IOException, ApiException, ParserConfigurationException, TransformerException, SAXException {
if (parentPath == null || parentPath.isBlank()) {
throw new IllegalArgumentException("Parent path must not be empty");
}
validateFileName(fileName);
String fullPath = parentPath.endsWith("/") ? parentPath + fileName : parentPath + "/" + fileName;
validateWithinProject(projectName, fullPath);
if (fileName.toLowerCase().endsWith(".xml")) {
configurationService.addConfigurationToFolder(projectName, fileName, parentPath);
} else {
fileSystemStorage.createFile(fullPath);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -56,16 +55,13 @@ public String updateConfiguration(String filepath, String content) | |||
| if (Files.isDirectory(absolutePath)) { | |||
| throw new ConfigurationNotFoundException("Invalid file path: " + filepath); | |||
| } | |||
| Document updatedDocument = XmlConfigurationUtils.insertFlowNamespace(content); | |||
| String updatedContent = XmlConfigurationUtils.convertNodeToString(updatedDocument); | |||
|
|
|||
| // Just write to the disk. ProjectService reads directly from disk now! | |||
| fileSystemStorage.writeFile(absolutePath.toString(), updatedContent); | |||
| return updatedContent; | |||
| fileSystemStorage.writeFile(absolutePath.toString(), content); | |||
| return true; | |||
There was a problem hiding this comment.
updateConfiguration now returns boolean but always returns true, and the controller ignores the return value. This makes the API misleading and adds unnecessary branching for callers/tests. Consider changing the method to void (or return something meaningful) and removing the unconditional return true.
| @PostMapping("/{projectName}/configurations/{configName}") | ||
| public ResponseEntity<ProjectDTO> addConfiguration( | ||
| @PathVariable String projectName, @PathVariable String configName) | ||
| throws ProjectNotFoundException, IOException { | ||
| throws ProjectNotFoundException, IOException, ParserConfigurationException, TransformerException, SAXException { | ||
| Project project = configurationService.addConfiguration(projectName, configName); |
There was a problem hiding this comment.
addConfiguration is now declared to throw TransformerException, but there is no global @ExceptionHandler for TransformerException (unlike SAXException / ParserConfigurationException). This will likely result in inconsistent/non-JSON 500 responses if a transform fails. Consider catching TransformerException here and wrapping it in ApiException (or adding a global handler) to keep error responses consistent.
| return apiFetch<void>(`/projects/${encodeURIComponent(projectName)}/configuration`, { | ||
| method: 'PUT', | ||
| body: JSON.stringify({ filepath, content }), | ||
| }) |
There was a problem hiding this comment.
fetchConfigurationCached uses the module-level configCache, but saveConfiguration doesn't update or invalidate that cache. After saving, other screens that rely on fetchConfigurationCached can read stale XML until a manual cache clear happens. Consider updating configCache for the saved key (set it to content) or deleting that key once the PUT succeeds.
| return apiFetch<void>(`/projects/${encodeURIComponent(projectName)}/configuration`, { | |
| method: 'PUT', | |
| body: JSON.stringify({ filepath, content }), | |
| }) | |
| await apiFetch<void>(`/projects/${encodeURIComponent(projectName)}/configuration`, { | |
| method: 'PUT', | |
| body: JSON.stringify({ filepath, content }), | |
| }) | |
| const key = `${projectName}:${filepath}` | |
| configCache.set(key, content) |
|



Now the code is not formatted or normalized when the file is getting saved.
Instead of moving elements, attributes and cursor:
Now the cursor stays where it was left and attributes dont shift
Note: