Skip to content

Commit 59ed4f5

Browse files
authored
Fix automate unrunnable tools (#7311)
# Description of Changes Fix automate unrunnable tools ## Problem - Remove Image failed in Automate with `Tool operation not supported: removeImage` - Its registry entry had `operationConfig: undefined` even though the config existed and was already tested - The Automate picker only filtered on `supportsAutomate`, never on `operationConfig` — so broken tools were selectable and failed only at run time ## Fixes - Wire up `removeImage` and `pageLayout` operation configs (both already existed, just never registered) - Exclude `validateSignature` (report tool, not on the operationConfig seam) and `scannerEffect` (no frontend implementation) via `supportsAutomate: false` - Picker now also filters on `operationConfig`, so this class of bug can't reach users again - `overlay-pdfs` returns 400 instead of 500 when overlay files or mode are missing - Fix `new URL().pathname` Windows path bug that stopped 2 test suites from loading --- ## Checklist ### General - [ ] I have read the [Contribution Guidelines](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/CONTRIBUTING.md) - [ ] I have read the [Stirling-PDF Developer Guide](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/DeveloperGuide.md) (if applicable) - [ ] I have read the [How to add new languages to Stirling-PDF](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/devGuide/HowToAddNewLanguage.md) (if applicable) - [ ] I have performed a self-review of my own code - [ ] My changes generate no new warnings ### Documentation - [ ] I have updated relevant docs on [Stirling-PDF's doc repo](https://github.com/Stirling-Tools/Stirling-Tools.github.io/blob/main/docs/) (if functionality has heavily changed) - [ ] I have read the section [Add New Translation Tags](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/devGuide/HowToAddNewLanguage.md#add-new-translation-tags) (for new translation tags only) ### Translations (if applicable) - [ ] I ran [`scripts/counter_translation.py`](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/docs/counter_translation.md) ### UI Changes (if applicable) - [ ] Screenshots or videos demonstrating the UI changes are attached (e.g., as comments or direct attachments in the PR) ### Testing (if applicable) - [ ] I have run `task check` to verify linters, typechecks, and tests pass - [ ] I have tested my changes locally. Refer to the [Testing Guide](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/DeveloperGuide.md#7-testing) for more details.
1 parent 7bf18cc commit 59ed4f5

6 files changed

Lines changed: 77 additions & 5 deletions

File tree

app/core/src/main/java/stirling/software/SPDF/controller/api/PdfOverlayController.java

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@ public ResponseEntity<Resource> overlayPdfs(@ModelAttribute OverlayPdfsRequest r
6161
int overlayPos = request.getOverlayPosition();
6262

6363
MultipartFile[] overlayFiles = request.getOverlayFiles();
64+
validateOverlayFiles(overlayFiles);
6465
File[] overlayPdfFiles = new File[overlayFiles.length];
6566
List<File> tempFiles = new ArrayList<>(); // List to keep track of temporary files
6667

@@ -120,10 +121,29 @@ public ResponseEntity<Resource> overlayPdfs(@ModelAttribute OverlayPdfsRequest r
120121
}
121122
}
122123

124+
// Both fields are declared required, but @ModelAttribute binding leaves them null when the
125+
// caller omits them, which would otherwise surface as a 500 instead of a 400.
126+
private void validateOverlayFiles(MultipartFile[] overlayFiles) {
127+
if (overlayFiles == null || overlayFiles.length == 0) {
128+
throw ExceptionUtils.createIllegalArgumentException(
129+
"error.overlayFilesRequired", "At least one overlay file is required");
130+
}
131+
for (MultipartFile overlayFile : overlayFiles) {
132+
if (overlayFile == null || overlayFile.isEmpty()) {
133+
throw ExceptionUtils.createIllegalArgumentException(
134+
"error.overlayFileEmpty", "Overlay files must not be empty");
135+
}
136+
}
137+
}
138+
123139
private Map<Integer, String> prepareOverlayGuide(
124140
int basePageCount, File[] overlayFiles, String mode, int[] counts, List<File> tempFiles)
125141
throws IOException {
126142
Map<Integer, String> overlayGuide = new HashMap<>();
143+
if (mode == null) {
144+
throw ExceptionUtils.createIllegalArgumentException(
145+
"error.invalidFormat", "Invalid {0} format: {1}", "overlay mode", "null");
146+
}
127147
switch (mode) {
128148
case "SequentialOverlay":
129149
sequentialOverlay(overlayGuide, overlayFiles, basePageCount, tempFiles);

frontend/editor/src/core/components/tools/automate/ToolSelector.tsx

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,13 +34,17 @@ export default function ToolSelector({
3434
const [shouldAutoFocus, setShouldAutoFocus] = useState(false);
3535
const containerRef = useRef<HTMLDivElement>(null);
3636

37-
// Filter out excluded tools (like 'automate' itself) and tools that don't support automation
37+
// Filter out excluded tools (like 'automate' itself), tools that don't support
38+
// automation, and tools with no operationConfig - the executor resolves a step
39+
// through operationConfig, so offering one without it fails only at run time.
3840
const baseFilteredTools = useMemo(() => {
3941
return (
4042
Object.entries(toolRegistry) as [ToolId, ToolRegistryEntry][]
4143
).filter(
4244
([key, tool]) =>
43-
!excludeTools.includes(key) && getToolSupportsAutomate(tool),
45+
!excludeTools.includes(key) &&
46+
getToolSupportsAutomate(tool) &&
47+
Boolean(tool.operationConfig),
4448
);
4549
}, [toolRegistry, excludeTools]);
4650

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
/**
2+
* Registry invariant: the Automate picker offers a tool whenever it doesn't opt out via
3+
* `supportsAutomate: false`, but automationExecutor resolves each step through the tool's
4+
* `operationConfig`. A tool that is offered without one is selectable in the builder and
5+
* only fails when the automation runs, with "Tool operation not supported: <toolId>".
6+
*
7+
* So a tool must either carry an operationConfig or declare supportsAutomate: false.
8+
*/
9+
import { describe, expect, test, vi } from "vitest";
10+
import { renderHook } from "@testing-library/react";
11+
import { useTranslatedToolCatalog } from "@app/data/useTranslatedToolRegistry";
12+
import { getToolSupportsAutomate } from "@app/data/toolsTaxonomy";
13+
14+
vi.mock("react-i18next", () => ({
15+
useTranslation: () => ({
16+
t: (key: string, fallback?: string) => fallback ?? key,
17+
i18n: { changeLanguage: vi.fn(), language: "en-US" },
18+
}),
19+
Trans: ({ children }: { children?: unknown }) => children,
20+
}));
21+
22+
describe("automatable tools", () => {
23+
test("every tool offered to Automate can be executed as a step", () => {
24+
const { result } = renderHook(() => useTranslatedToolCatalog());
25+
26+
const offeredWithoutConfig = Object.entries(result.current.regularTools)
27+
.filter(([, entry]) => entry && getToolSupportsAutomate(entry))
28+
.filter(([, entry]) => !entry.operationConfig)
29+
.map(([id]) => id);
30+
31+
expect(offeredWithoutConfig).toEqual([]);
32+
});
33+
});

frontend/editor/src/core/data/useTranslatedToolRegistry.tsx

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,8 @@ import { changeMetadataOperationConfig } from "@app/hooks/tools/changeMetadata/u
5050
import { signOperationConfig } from "@app/hooks/tools/sign/useSignOperation";
5151
import { cropOperationConfig } from "@app/hooks/tools/crop/useCropOperation";
5252
import { removeAnnotationsOperationConfig } from "@app/hooks/tools/removeAnnotations/useRemoveAnnotationsOperation";
53+
import { removeImageOperationConfig } from "@app/hooks/tools/removeImage/useRemoveImageOperation";
54+
import { pageLayoutOperationConfig } from "@app/hooks/tools/pageLayout/usePageLayoutOperation";
5355
import { extractImagesOperationConfig } from "@app/hooks/tools/extractImages/useExtractImagesOperation";
5456
import { replaceColorOperationConfig } from "@app/hooks/tools/replaceColor/useReplaceColorOperation";
5557
import { removePagesOperationConfig } from "@app/hooks/tools/removePages/useRemovePagesOperation";
@@ -526,6 +528,9 @@ export function useTranslatedToolCatalog(): TranslatedToolCatalog {
526528
maxFiles: -1,
527529
endpoints: ["validate-signature"],
528530
synonyms: getSynonyms(t, "validateSignature"),
531+
// Reports on signatures rather than transforming the PDF, and its hook is
532+
// not on the operationConfig seam, so it cannot run as an automation step.
533+
supportsAutomate: false,
529534
automationSettings: null,
530535
},
531536

@@ -755,6 +760,7 @@ export function useTranslatedToolCatalog(): TranslatedToolCatalog {
755760
subcategoryId: SubcategoryId.PAGE_FORMATTING,
756761
maxFiles: -1,
757762
endpoints: ["multi-page-layout"],
763+
operationConfig: asRegistryConfig(pageLayoutOperationConfig),
758764
automationSettings: lazySettings(
759765
() => import("@app/components/tools/pageLayout/PageLayoutSettings"),
760766
),
@@ -967,7 +973,7 @@ export function useTranslatedToolCatalog(): TranslatedToolCatalog {
967973
subcategoryId: SubcategoryId.REMOVAL,
968974
maxFiles: -1,
969975
endpoints: ["remove-image-pdf"],
970-
operationConfig: undefined,
976+
operationConfig: asRegistryConfig(removeImageOperationConfig),
971977
synonyms: getSynonyms(t, "removeImage"),
972978
automationSettings: null,
973979
},
@@ -1196,6 +1202,9 @@ export function useTranslatedToolCatalog(): TranslatedToolCatalog {
11961202
subcategoryId: SubcategoryId.ADVANCED_FORMATTING,
11971203
endpoints: ["scanner-effect"],
11981204
synonyms: getSynonyms(t, "scannerEffect"),
1205+
// No frontend implementation yet (component is null), so it has no
1206+
// operationConfig to execute as an automation step.
1207+
supportsAutomate: false,
11991208
automationSettings: null,
12001209
},
12011210

frontend/editor/src/core/utils/toolIOCompat.test.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import { readFileSync } from "node:fs";
44
import { dirname, resolve } from "node:path";
5+
import { fileURLToPath } from "node:url";
56
import { describe, expect, it } from "vitest";
67
import {
78
validateToolChain,
@@ -24,7 +25,9 @@ interface SharedCase {
2425

2526
/** Shared with the backend and engine, so it lives at the repo root. */
2627
function casesFile(): string {
27-
let current = dirname(new URL(import.meta.url).pathname);
28+
// fileURLToPath, not URL.pathname: on Windows the latter yields "/C:/..." and
29+
// resolving against it produces a "C:\C:\..." path that never matches.
30+
let current = dirname(fileURLToPath(import.meta.url));
2831
for (let i = 0; i < 12; i++) {
2932
const candidate = resolve(current, "testing/tool-io-cases.json");
3033
try {

frontend/editor/src/core/utils/toolIOLabels.test.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { readFileSync } from "node:fs";
22
import { dirname, resolve } from "node:path";
3+
import { fileURLToPath } from "node:url";
34
import { describe, expect, it } from "vitest";
45
import { TOOL_FORMATS, type ToolFormat } from "@app/types/toolIO";
56
import {
@@ -9,7 +10,9 @@ import {
910

1011
/** The en-US `[toolFormat]` block, read straight from the locale file. */
1112
function toolFormatLabels(): Record<string, string> {
12-
let current = dirname(new URL(import.meta.url).pathname);
13+
// fileURLToPath, not URL.pathname: on Windows the latter yields "/C:/..." and
14+
// resolving against it produces a "C:\C:\..." path that never matches.
15+
let current = dirname(fileURLToPath(import.meta.url));
1316
for (let i = 0; i < 12; i++) {
1417
try {
1518
const toml = readFileSync(

0 commit comments

Comments
 (0)