feat: add upload_image MCP tool and screenshot support#3574
Open
feat: add upload_image MCP tool and screenshot support#3574
Conversation
Add a new `upload_image` MCP tool that accepts base64-encoded images and returns an image ID. Enhance `create_keys` to optionally accept screenshot references (uploadedImageId + positions) per key, associating uploaded images with keys after creation. Also rewrite webapp/CLAUDE.md to use MCP tools instead of curl, clarify that defaultValue is temporary (for screenshots only), make screenshots mandatory (opt-out), and add app startup/shutdown instructions.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds MCP screenshot capabilities: new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as MCP Client
participant UploadTool as upload_image Tool
participant ImageService as ImageUploadService
participant CreateKeysTool as create_keys Tool
participant KeyService as KeyService
participant ScreenshotService as ScreenshotService
Client->>UploadTool: upload_image(base64)
UploadTool->>UploadTool: validate size & decode
UploadTool->>ImageService: store(imageBytes)
ImageService-->>UploadTool: imageId
UploadTool-->>Client: { "imageId": id }
Client->>CreateKeysTool: create_keys(keys with screenshots)
CreateKeysTool->>KeyService: importKeys(keyDtos)
KeyService-->>CreateKeysTool: created key list
loop per key with screenshots
CreateKeysTool->>KeyService: find(resolvedKey)
KeyService-->>CreateKeysTool: keyEntity
CreateKeysTool->>ScreenshotService: saveUploadedImagesForKeys((keyEntity, screenshotDtos))
ScreenshotService-->>CreateKeysTool: associationsCreated
end
CreateKeysTool-->>Client: { "created": true, "keyCount": n }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Add a dedicated tool for associating screenshots with keys that already exist, complementing the create_keys screenshot support which only works during key creation. Also mention the new tool in webapp/CLAUDE.md.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
backend/app/src/test/kotlin/io/tolgee/mcp/tools/McpScreenshotToolsTest.kt (1)
166-205: Consider verifying that position data is actually persisted.The test confirms that
create_keyswith positions doesn't fail and screenshots are associated, but it doesn't verify that the position coordinates (x=10, y=20, width=100, height=30) are actually stored. This could mask a bug where positions are silently dropped.💡 Suggested: Add position verification
val key = keyService.find(data.projectId, "positioned.key", null) assertThat(key).isNotNull() val screenshots = screenshotService.findAll(key!!) assertThat(screenshots).isNotEmpty() + + // Verify position data was stored + val keyInScreenshots = screenshots.first().keyScreenshotReferences + assertThat(keyInScreenshots).isNotEmpty() + val position = keyInScreenshots.first() + assertThat(position.x).isEqualTo(10) + assertThat(position.y).isEqualTo(20) + assertThat(position.width).isEqualTo(100) + assertThat(position.height).isEqualTo(30)Note: Adjust field access based on the actual entity structure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/src/test/kotlin/io/tolgee/mcp/tools/McpScreenshotToolsTest.kt` around lines 166 - 205, The test create_keys with screenshots and positions stores position data currently only checks screenshots exist but not that the position coordinates were persisted; update the test (McpScreenshotToolsTest::`create_keys with screenshots and positions stores position data`) to fetch positions from the persisted screenshot(s) (via screenshotService.findAll(key) and the screenshot entity's positions collection or a Position/KeyPosition service) and assert that there is one position with x==10, y==20, width==100 and height==30 (use the actual entity getters or fields present in your domain model to read the stored values).backend/app/src/main/kotlin/io/tolgee/mcp/tools/KeyMcpTools.kt (1)
164-192: Consider logging when a key is not found during screenshot association.The
continueon line 173 silently skips screenshot association if the key is not found. While this may be intentional (e.g., for keys that existed before and were skipped byimportKeys), silent failures can make debugging difficult. Consider adding a log statement when a key is unexpectedly not found.💡 Optional: Add debug logging
val keyEntity = keyService.find(projectHolder.project.id, keyName, keyNamespace, branch) - ?: continue + ?: run { + // Key may have been skipped if it already existed + continue + }Or add a logger call if the class has a logger configured.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/src/main/kotlin/io/tolgee/mcp/tools/KeyMcpTools.kt` around lines 164 - 192, In KeyMcpTools, when iterating keysWithScreenshots inside the executeInNewTransaction block, add a log statement before the existing continue where keyService.find(...) returns null: use the class logger (e.g., logger.debug or logger.warn) to record keyName, keyNamespace (or defaultNamespace), branch and projectHolder.project.id so missing-key cases are visible; keep the continue behavior but ensure the message clearly indicates screenshots were skipped for that key to aid debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/app/src/main/kotlin/io/tolgee/mcp/tools/KeyMcpTools.kt`:
- Around line 164-192: In KeyMcpTools, when iterating keysWithScreenshots inside
the executeInNewTransaction block, add a log statement before the existing
continue where keyService.find(...) returns null: use the class logger (e.g.,
logger.debug or logger.warn) to record keyName, keyNamespace (or
defaultNamespace), branch and projectHolder.project.id so missing-key cases are
visible; keep the continue behavior but ensure the message clearly indicates
screenshots were skipped for that key to aid debugging.
In `@backend/app/src/test/kotlin/io/tolgee/mcp/tools/McpScreenshotToolsTest.kt`:
- Around line 166-205: The test create_keys with screenshots and positions
stores position data currently only checks screenshots exist but not that the
position coordinates were persisted; update the test
(McpScreenshotToolsTest::`create_keys with screenshots and positions stores
position data`) to fetch positions from the persisted screenshot(s) (via
screenshotService.findAll(key) and the screenshot entity's positions collection
or a Position/KeyPosition service) and assert that there is one position with
x==10, y==20, width==100 and height==30 (use the actual entity getters or fields
present in your domain model to read the stored values).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 90b02b5a-8e9f-4d3e-b334-f25b16c5649a
📒 Files selected for processing (5)
backend/app/src/main/kotlin/io/tolgee/mcp/tools/KeyMcpTools.ktbackend/app/src/main/kotlin/io/tolgee/mcp/tools/McpToolUtils.ktbackend/app/src/main/kotlin/io/tolgee/mcp/tools/ScreenshotMcpTools.ktbackend/app/src/test/kotlin/io/tolgee/mcp/tools/McpScreenshotToolsTest.ktwebapp/CLAUDE.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/app/src/test/kotlin/io/tolgee/mcp/tools/McpScreenshotToolsTest.kt (1)
166-205: Test doesn't verify position data is actually stored.The test name says "stores position data" but only asserts
screenshots.isNotEmpty(). Consider verifying the actual position values were persisted.♻️ Suggested enhancement
val key = keyService.find(data.projectId, "positioned.key", null) assertThat(key).isNotNull() val screenshots = screenshotService.findAll(key!!) assertThat(screenshots).isNotEmpty() + + // Verify position data was stored + val keyReferences = screenshots.first().keyScreenshotReferences + assertThat(keyReferences).isNotEmpty() + val position = keyReferences.first() + assertThat(position.x).isEqualTo(10) + assertThat(position.y).isEqualTo(20) + assertThat(position.width).isEqualTo(100) + assertThat(position.height).isEqualTo(30)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/src/test/kotlin/io/tolgee/mcp/tools/McpScreenshotToolsTest.kt` around lines 166 - 205, The test create_keys with screenshots and positions stores position data currently only checks screenshots are present; extend it to fetch the persisted Screenshot (via screenshotService.findAll(key) or screenshotService.findById) and assert the stored position fields match the input (x=10, y=20, width=100, height=30). Use the existing keyService.find(...) result to get the screenshots list, extract the position object (e.g., screenshot.positions or screenshot.position), and add assertions that each numeric field equals the expected values so the test verifies the actual persisted position data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/src/main/kotlin/io/tolgee/mcp/tools/ScreenshotMcpTools.kt`:
- Around line 117-148: The loop inside executeInNewTransaction currently returns
early with errorResult when keyService.find fails, which exits the lambda
without rolling back after some screenshots have been saved; to fix, first
pre-validate all keys before any mutations by iterating keyScreenshots and
calling keyService.find(projectHolder.project.id, keyName, keyNamespace, branch)
for each, collecting missing keys and returning errorResult if any are absent,
then in a second pass map screenshots and call
screenshotService.saveUploadedImages only after validation succeeds;
alternatively, replace the early return with throwing a runtime exception (e.g.,
IllegalStateException) instead of return@executeInNewTransaction so the
transaction is rolled back—apply this change to the code handling
keyScreenshots, keyService.find, and screenshotService.saveUploadedImages.
In `@webapp/CLAUDE.md`:
- Around line 100-103: The README currently shows the macOS-only base64 usage
with the -i flag; update the CLAUDE.md snippet to use portable base64 invocation
by removing the macOS-specific -i flag and instead demonstrate the two
cross-platform options: passing the filename as an argument or piping the file
into base64 via stdin, and explicitly note that -i is macOS-only while Linux
accepts the filename or stdin approach; adjust the surrounding text to document
both variants so readers on different platforms can pick the correct form.
---
Nitpick comments:
In `@backend/app/src/test/kotlin/io/tolgee/mcp/tools/McpScreenshotToolsTest.kt`:
- Around line 166-205: The test create_keys with screenshots and positions
stores position data currently only checks screenshots are present; extend it to
fetch the persisted Screenshot (via screenshotService.findAll(key) or
screenshotService.findById) and assert the stored position fields match the
input (x=10, y=20, width=100, height=30). Use the existing keyService.find(...)
result to get the screenshots list, extract the position object (e.g.,
screenshot.positions or screenshot.position), and add assertions that each
numeric field equals the expected values so the test verifies the actual
persisted position data.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: daf3de7a-545d-4ab8-936d-500d42171b8f
📒 Files selected for processing (3)
backend/app/src/main/kotlin/io/tolgee/mcp/tools/ScreenshotMcpTools.ktbackend/app/src/test/kotlin/io/tolgee/mcp/tools/McpScreenshotToolsTest.ktwebapp/CLAUDE.md
Comment on lines
+100
to
+103
| 1. Base64-encode the screenshot file: | ||
| ```bash | ||
| base64 -i <screenshot-file> | ||
| ``` |
Contributor
There was a problem hiding this comment.
Platform-specific base64 command syntax.
The -i flag is macOS-specific. On Linux, base64 reads from stdin or accepts the filename directly without -i.
Consider a more portable approach:
- base64 -i <screenshot-file>
+ base64 < <screenshot-file>Or document both variants explicitly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 1. Base64-encode the screenshot file: | |
| ```bash | |
| base64 -i <screenshot-file> | |
| ``` | |
| 1. Base64-encode the screenshot file: |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webapp/CLAUDE.md` around lines 100 - 103, The README currently shows the
macOS-only base64 usage with the -i flag; update the CLAUDE.md snippet to use
portable base64 invocation by removing the macOS-specific -i flag and instead
demonstrate the two cross-platform options: passing the filename as an argument
or piping the file into base64 via stdin, and explicitly note that -i is
macOS-only while Linux accepts the filename or stdin approach; adjust the
surrounding text to document both variants so readers on different platforms can
pick the correct form.
The hand-crafted PNG bytes had incorrect zlib compression, causing "Error skipping PNG metadata" in image processing. Replace with a properly generated 1x1 PNG base64 constant.
saveUploadedImages deletes the uploaded image after converting it to a screenshot. When the same image is referenced by multiple keys, track already-processed images and use addReference for subsequent keys instead of calling saveUploadedImages again.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
backend/app/src/main/kotlin/io/tolgee/mcp/tools/ScreenshotMcpTools.kt (1)
120-128:⚠️ Potential issue | 🟠 Major
errorResulthere does not roll back earlier screenshot writes.
return@executeInNewTransactionexits the lambda normally, so any screenshots saved for previous entries can still commit before the error is returned. Pre-resolve all keys before the mutation loop, or throw so the transaction aborts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/src/main/kotlin/io/tolgee/mcp/tools/ScreenshotMcpTools.kt` around lines 120 - 128, The current executeInNewTransaction lambda returns early with return@executeInNewTransaction when keyService.find fails, which does not abort the transaction and allows prior screenshot writes to commit; change the flow to pre-resolve all keys before performing any mutations (iterate keyScreenshots first, call keyService.find for each and collect resolved Key entities or stop), or instead throw an exception (e.g., IllegalStateException) when a key is missing so the transaction will roll back; ensure the code paths referencing keyScreenshots, keyService.find, executeInNewTransaction, and errorResult are updated so mutations only happen after successful pre-resolution or use exceptions to abort the transaction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/src/main/kotlin/io/tolgee/mcp/tools/KeyMcpTools.kt`:
- Around line 165-217: The two-step flow is non-atomic:
keyService.importKeys(...) runs before the screenshot pass and re-resolves keys
which can cause duplicate/incorrect attachments on retry; fix by performing
importKeys and the screenshot association inside the same transaction (use
executeInNewTransaction(transactionManager) to wrap both) and by only processing
keys that were created by this import call (capture the import result / created
key IDs returned by keyService.importKeys and use that set to filter
rawKeys/screenshots instead of re-resolving by name). Ensure you call
screenshotService.saveUploadedImages/addReference only for keyEntity objects
belonging to the created-key set and keep the processedImages memoization logic
as-is to avoid double-conversion.
- Around line 167-217: The importKeys endpoint currently allows screenshot
uploads because KeyMcpTools calls screenshotService.saveUploadedImages() without
checking SCREENSHOTS_UPLOAD; update the permission model by adding
Scope.SCREENSHOTS_UPLOAD to the `@RequiresProjectPermissions` annotation on
KeyController.importKeys or, alternatively, add an explicit permission check in
KeyMcpTools before processing screenshots (check project permissions for
Scope.SCREENSHOTS_UPLOAD and abort/throw if missing) so only users with
SCREENSHOTS_UPLOAD can call
screenshotService.saveUploadedImages()/screenshotService.addReference.
---
Duplicate comments:
In `@backend/app/src/main/kotlin/io/tolgee/mcp/tools/ScreenshotMcpTools.kt`:
- Around line 120-128: The current executeInNewTransaction lambda returns early
with return@executeInNewTransaction when keyService.find fails, which does not
abort the transaction and allows prior screenshot writes to commit; change the
flow to pre-resolve all keys before performing any mutations (iterate
keyScreenshots first, call keyService.find for each and collect resolved Key
entities or stop), or instead throw an exception (e.g., IllegalStateException)
when a key is missing so the transaction will roll back; ensure the code paths
referencing keyScreenshots, keyService.find, executeInNewTransaction, and
errorResult are updated so mutations only happen after successful pre-resolution
or use exceptions to abort the transaction.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 78604f2f-ad85-475c-803b-c26fb6847cf3
📒 Files selected for processing (2)
backend/app/src/main/kotlin/io/tolgee/mcp/tools/KeyMcpTools.ktbackend/app/src/main/kotlin/io/tolgee/mcp/tools/ScreenshotMcpTools.kt
Comment on lines
165
to
+217
| keyService.importKeys(keys, projectHolder.projectEntity, branch) | ||
|
|
||
| // Associate screenshots with keys (post-processing after key creation) | ||
| // Track already-processed uploaded images: saveUploadedImages deletes the | ||
| // uploaded image after converting it to a screenshot, so when the same | ||
| // uploadedImageId is referenced by multiple keys we must use addReference | ||
| // for subsequent keys instead of saveUploadedImages again. | ||
| val keysWithScreenshots = rawKeys.filter { it.getList("screenshots") != null } | ||
| if (keysWithScreenshots.isNotEmpty()) { | ||
| executeInNewTransaction(transactionManager) { | ||
| val processedImages = mutableMapOf<Long, Pair<Screenshot, Pair<Dimension?, Dimension?>>>() | ||
| for (rawKey in keysWithScreenshots) { | ||
| val keyName = rawKey.requireString("name") | ||
| val keyNamespace = rawKey.getString("namespace") ?: defaultNamespace | ||
| val keyEntity = | ||
| keyService.find(projectHolder.project.id, keyName, keyNamespace, branch) | ||
| ?: continue | ||
| for (s in rawKey.requireList("screenshots")) { | ||
| val imageId = s.requireLong("uploadedImageId") | ||
| val positions = | ||
| s.getList("positions")?.map { p -> | ||
| KeyInScreenshotPositionDto( | ||
| x = p.requireInt("x"), | ||
| y = p.requireInt("y"), | ||
| width = p.requireInt("width"), | ||
| height = p.requireInt("height"), | ||
| ) | ||
| } | ||
| val existing = processedImages[imageId] | ||
| if (existing != null) { | ||
| // Image already converted to screenshot — just add a reference | ||
| val (screenshot, dims) = existing | ||
| val info = ScreenshotInfoDto(positions = positions) | ||
| screenshotService.addReference(keyEntity, screenshot, info, dims.first, dims.second) | ||
| } else { | ||
| // First time seeing this image — convert and track it | ||
| val dto = | ||
| listOf( | ||
| KeyScreenshotDto().apply { | ||
| uploadedImageId = imageId | ||
| this.positions = positions | ||
| }, | ||
| ) | ||
| val result = screenshotService.saveUploadedImages(dto, keyEntity) | ||
| val screenshot = result.values.first() | ||
| processedImages[imageId] = | ||
| screenshot to | ||
| (Dimension(screenshot.width, screenshot.height) to Dimension(screenshot.width, screenshot.height)) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Keep create_keys plus screenshot attachment atomic.
importKeys() finishes before this second pass starts, and the pass re-resolves keys by name. If a later screenshot save fails, the keys are already created; on retry, the same logic can then attach screenshots to pre-existing keys that create_keys is supposed to skip. Run both steps in one transaction and only process keys created by this call.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/src/main/kotlin/io/tolgee/mcp/tools/KeyMcpTools.kt` around lines
165 - 217, The two-step flow is non-atomic: keyService.importKeys(...) runs
before the screenshot pass and re-resolves keys which can cause
duplicate/incorrect attachments on retry; fix by performing importKeys and the
screenshot association inside the same transaction (use
executeInNewTransaction(transactionManager) to wrap both) and by only processing
keys that were created by this import call (capture the import result / created
key IDs returned by keyService.importKeys and use that set to filter
rawKeys/screenshots instead of re-resolving by name). Ensure you call
screenshotService.saveUploadedImages/addReference only for keyEntity objects
belonging to the created-key set and keep the processedImages memoization logic
as-is to avoid double-conversion.
Move the multi-key screenshot association logic into a single ScreenshotService.saveUploadedImagesForKeys() method. This eliminates duplicated image-tracking logic in both KeyMcpTools and ScreenshotMcpTools — both now call the shared service method. Also extract parseScreenshotDtos() helper for MCP argument parsing.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
backend/app/src/main/kotlin/io/tolgee/mcp/tools/KeyMcpTools.kt (1)
160-176:⚠️ Potential issue | 🟠 Major
create_keysno longer honors the "skip existing" contract for screenshots.After
importKeys(), the second pass re-resolves keys by name and attaches screenshots even when the key already existed and was skipped. Because that pass runs separately, a later screenshot failure also leaves the key creation committed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/src/main/kotlin/io/tolgee/mcp/tools/KeyMcpTools.kt` around lines 160 - 176, The second-pass screenshot attach step re-resolves keys and attaches screenshots even for keys that were skipped by keyService.importKeys(), breaking the "skip existing" contract and leaving commits if screenshot save fails; fix by making the screenshot pass only operate on keys actually created in the import (or by performing screenshot attachment inside the same import transaction). Concretely: change keyService.importKeys(...) to return the identifiers or KeyEntity instances of keys it created (or add a flag per key), then in the screenshot block filter rawKeys against that returned set instead of re-resolving by name (use the returned created key IDs/objects to build keyScreenshotPairs for parseScreenshotDtos and pass those to screenshotService.saveUploadedImagesForKeys), or move parseScreenshotDtos + saveUploadedImagesForKeys into importKeys so the whole operation is atomic under executeInNewTransaction/transactionManager.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/src/main/kotlin/io/tolgee/mcp/tools/ScreenshotMcpTools.kt`:
- Around line 117-126: The code builds keyScreenshotPairs using
keyService.find(...) and then saves screenshots without enforcing branch write
permissions; call securityService.checkBranchModify(keyEntity) for each resolved
key inside the same executeInNewTransaction block (before adding to
keyScreenshotPairs and before calling
screenshotService.saveUploadedImagesForKeys) and return the appropriate
errorResult if the check fails so that
KeyScreenshotController.uploadScreenshot's branch-level authorization is
reapplied; reference the functions/variables keyService.find,
securityService.checkBranchModify, executeInNewTransaction, keyScreenshotPairs,
and screenshotService.saveUploadedImagesForKeys when making the change.
In `@backend/data/src/main/kotlin/io/tolgee/service/key/ScreenshotService.kt`:
- Around line 245-273: The code currently converts and deletes uploaded images
via saveScreenshot(image) before verifying that all requested uploadedImageIds
exist and that each key will not exceed maxScreenshotsPerKey; change
saveUploadedImagesForKeys to first validate: (1) collect allImageIds from
keyScreenshots and ensure imageUploadService.find(allImageIds) returns an image
for every id (throw NotFoundException if any missing), and verify ownership of
each image (existing check) before any saveScreenshot calls; (2) for each Key in
keyScreenshots compute the number of distinct incoming screenshot IDs for that
key and fetch the current screenshot count for the key (use the repository or
existing count method used by store()) and throw a BadRequest/Quota exception if
current + incoming would exceed maxScreenshotsPerKey; only after these
validations proceed to call saveScreenshot for each image and then
addReference/store as before.
---
Duplicate comments:
In `@backend/app/src/main/kotlin/io/tolgee/mcp/tools/KeyMcpTools.kt`:
- Around line 160-176: The second-pass screenshot attach step re-resolves keys
and attaches screenshots even for keys that were skipped by
keyService.importKeys(), breaking the "skip existing" contract and leaving
commits if screenshot save fails; fix by making the screenshot pass only operate
on keys actually created in the import (or by performing screenshot attachment
inside the same import transaction). Concretely: change
keyService.importKeys(...) to return the identifiers or KeyEntity instances of
keys it created (or add a flag per key), then in the screenshot block filter
rawKeys against that returned set instead of re-resolving by name (use the
returned created key IDs/objects to build keyScreenshotPairs for
parseScreenshotDtos and pass those to
screenshotService.saveUploadedImagesForKeys), or move parseScreenshotDtos +
saveUploadedImagesForKeys into importKeys so the whole operation is atomic under
executeInNewTransaction/transactionManager.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 99fd5a2e-21b7-4f2b-9ea6-c50bf9f54dd9
📒 Files selected for processing (3)
backend/app/src/main/kotlin/io/tolgee/mcp/tools/KeyMcpTools.ktbackend/app/src/main/kotlin/io/tolgee/mcp/tools/ScreenshotMcpTools.ktbackend/data/src/main/kotlin/io/tolgee/service/key/ScreenshotService.kt
Comment on lines
+117
to
+126
| executeInNewTransaction(transactionManager) { | ||
| val keyScreenshotPairs = | ||
| keyScreenshots.map { entry -> | ||
| val keyName = entry.requireString("keyName") | ||
| val keyEntity = | ||
| keyService.find(projectHolder.project.id, keyName, entry.getString("namespace"), branch) | ||
| ?: return@executeInNewTransaction errorResult("Key not found: $keyName") | ||
| keyEntity to parseScreenshotDtos(entry.requireList("screenshots")) | ||
| } | ||
| screenshotService.saveUploadedImagesForKeys(keyScreenshotPairs) |
Contributor
There was a problem hiding this comment.
Reapply checkBranchModify before adding screenshots.
buildSpec(KeyScreenshotController::uploadScreenshot, ...) carries the controller metadata, but it does not execute KeyScreenshotController.uploadScreenshot()'s securityService.checkBranchModify(keyEntity). This tool currently mutates screenshot references after only a keyService.find(...), so branch-level write restrictions are bypassed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/src/main/kotlin/io/tolgee/mcp/tools/ScreenshotMcpTools.kt` around
lines 117 - 126, The code builds keyScreenshotPairs using keyService.find(...)
and then saves screenshots without enforcing branch write permissions; call
securityService.checkBranchModify(keyEntity) for each resolved key inside the
same executeInNewTransaction block (before adding to keyScreenshotPairs and
before calling screenshotService.saveUploadedImagesForKeys) and return the
appropriate errorResult if the check fails so that
KeyScreenshotController.uploadScreenshot's branch-level authorization is
reapplied; reference the functions/variables keyService.find,
securityService.checkBranchModify, executeInNewTransaction, keyScreenshotPairs,
and screenshotService.saveUploadedImagesForKeys when making the change.
Review fixes:
- Add upload_image and add_key_screenshots to McpWithoutEeTest.coreTools
- Strengthen position assertion in McpScreenshotToolsTest
Boy scout improvements in ScreenshotService:
- Fix typo: adjustByRation → adjustByRatio
- Simplify forEach { delete(it) } → forEach(::delete)
- Fix shadowed 'it' variable in getScreenshotsForKeys
- Replace .toSet().toList() with .distinctBy { it.id }
- Remove unnecessary .let wrapper in saveUploadedImages
Boy scout improvement in McpToolUtils:
- Replace unused 'exchange' parameter with '_'
- Add SCREENSHOTS_UPLOAD scope check in create_keys before screenshot association (prevents scope escalation via PAK with only key/translation permissions) - Add eager validation of image IDs in saveUploadedImagesForKeys (fail fast before any mutations if images don't exist) - Add maxScreenshotsPerKey limit check in saveUploadedImagesForKeys (matching the existing check in store()) - Use portable base64 syntax in webapp/CLAUDE.md (base64 < file instead of macOS-specific base64 -i) - Add clarifying comment in add_key_screenshots key resolution
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
upload_imageMCP tool that accepts base64-encoded images and returns an image ID for later use withcreate_keyscreate_keysMCP tool to optionally acceptscreenshotsper key (withuploadedImageIdandpositions), associating uploaded images with keys after creationrequireIntutility toMcpToolUtilswebapp/CLAUDE.mdto use MCP tools instead of curl, clarifydefaultValueis temporary (for screenshots only), make screenshots mandatory (opt-out), and add app startup/shutdown instructionsSummary by CodeRabbit
New Features
Tests
Documentation