Open
Conversation
WalkthroughThreads an optional Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI (KeyDialog / KeyForm)
participant DialogCtx as DialogContext
participant QueryProv as QueryProvider
participant DevBackend as DevBackend
participant Core as Core/Plugins
UI->>DialogCtx: open dialog (uiProps.branch?)
DialogCtx->>QueryProv: set branchParam, readOnly=false
UI->>QueryProv: request translations (namespace, branch)
QueryProv->>DevBackend: getRecord(projectId?, branch)
DevBackend->>Core: resolve project (apiKey or projectId)
DevBackend->>DevBackend: append ?branch={branch}&ns={ns}
DevBackend-->>QueryProv: return record or error
alt branch_not_found or read-only error
DevBackend-->>DialogCtx: error
DialogCtx->>UI: set readOnly=true / show error
else success
QueryProv-->>UI: provide translations and branch info
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 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
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: 1
🤖 Fix all issues with AI agents
In `@packages/web/src/package/ui/KeyDialog/dialogContext/index.ts`:
- Around line 275-281: The URL construction for linkToPlatform interpolates
branchParam directly into the path (see linkToPlatform, branchParam,
props.uiProps.apiUrl, scopesLoadable.data.projectId, props.keyName, selectedNs);
change the code to percent-encode branchParam with encodeURIComponent() when
embedding it into the path segment, or if your server rejects encoded slashes
move branchParam out of the path and append it as a query parameter (e.g.,
?branch=encodedBranch) so the branch value is safely transmitted.
🧹 Nitpick comments (1)
packages/web/src/package/DevBackend.ts (1)
35-37: Consider moving validation earlier.The PAT key validation occurs after URL construction. Moving it before URL construction would avoid unnecessary work when validation fails.
♻️ Suggested reorder
function createDevBackend(): BackendDevMiddleware { return { getRecord({ apiUrl, apiKey, projectId, branch, language, namespace, filterTag, fetch, }) { + if (getApiKeyType(apiKey) === 'tgpat' && projectId === undefined) { + throw new Error("You need to specify 'projectId' when using PAT key"); + } + const pId = getProjectIdFromApiKey(apiKey) ?? projectId; let url: URL; // ... URL construction ... - - if (getApiKeyType(apiKey) === 'tgpat' && projectId === undefined) { - throw new Error("You need to specify 'projectId' when using PAT key"); - }
zomp
force-pushed
the
janmolnar/branching
branch
from
4e4fcec to
8ad0508
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/web/src/package/ui/KeyDialog/dialogContext/index.ts`:
- Around line 399-405: The handler stores read-only state via setReadOnly(true)
but still calls setSubmitError(e), causing duplicate ErrorAlert and the
read-only banner; change the logic in the error handling (the block that checks
e instanceof HttpError && e.code ===
'operation_not_permitted_in_read_only_mode') to setReadOnly(true) and skip
calling setSubmitError for that specific error code so only the read-only
info/banner is shown; retain setSubmitError(e) for all other errors.
Comment on lines
+399
to
405
| if ( | ||
| e instanceof HttpError && | ||
| e.code === 'operation_not_permitted_in_read_only_mode' | ||
| ) { | ||
| setReadOnly(true); | ||
| } | ||
| setSubmitError(e); |
There was a problem hiding this comment.
Avoid duplicate alerts for read-only submit errors.
When the read-only error occurs, readOnly is set but submitError is still stored, which can lead to a second ErrorAlert in the UI. Consider skipping submitError for this case so only the read-only info banner shows.
💡 Suggested fix
- if (
- e instanceof HttpError &&
- e.code === 'operation_not_permitted_in_read_only_mode'
- ) {
- setReadOnly(true);
- }
- setSubmitError(e);
+ if (
+ e instanceof HttpError &&
+ e.code === 'operation_not_permitted_in_read_only_mode'
+ ) {
+ setReadOnly(true);
+ setSubmitError(undefined);
+ } else {
+ setSubmitError(e);
+ }📝 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
| if ( | |
| e instanceof HttpError && | |
| e.code === 'operation_not_permitted_in_read_only_mode' | |
| ) { | |
| setReadOnly(true); | |
| } | |
| setSubmitError(e); | |
| if ( | |
| e instanceof HttpError && | |
| e.code === 'operation_not_permitted_in_read_only_mode' | |
| ) { | |
| setReadOnly(true); | |
| setSubmitError(undefined); | |
| } else { | |
| setSubmitError(e); | |
| } |
🤖 Prompt for AI Agents
In `@packages/web/src/package/ui/KeyDialog/dialogContext/index.ts` around lines
399 - 405, The handler stores read-only state via setReadOnly(true) but still
calls setSubmitError(e), causing duplicate ErrorAlert and the read-only banner;
change the logic in the error handling (the block that checks e instanceof
HttpError && e.code === 'operation_not_permitted_in_read_only_mode') to
setReadOnly(true) and skip calling setSubmitError for that specific error code
so only the read-only info/banner is shown; retain setSubmitError(e) for all
other errors.
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
3 participants
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.
Feature implementation probably finished. Tests not done yet. Waiting on tolgee/tolgee-platform#3246 merge.
Summary by CodeRabbit
New Features
Bug Fixes
Behavior
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.