83 fix darkmode colors and hidden layers #85
Conversation
WalkthroughAdds a dark-mode primary text CSS variable and applies it to CodeMirror placeholders; introduces optional ignoreHiddenLayersIncludingChildren (type, storage, UI) and updates node inclusion to optionally check hidden ancestors; refines Pull/Push error handling for uploaded images and screenshot/tag flows; removes an unused mutation import; small UI and z-index tweaks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant SettingsUI as Settings UI
participant SettingsTools as settingsTools
participant PluginStorage as Plugin Storage
participant NodeTools as nodeTools
Note over SettingsUI,NodeTools: Opt-in flag controls ancestor-aware filtering
SettingsUI->>SettingsTools: setTolgeeConfig(ignoreHiddenLayersIncludingChildren)
SettingsTools->>PluginStorage: setGlobalSettings(..., ignoreHiddenLayersIncludingChildren)
SettingsTools->>PluginStorage: setDocumentData(..., ignoreHiddenLayersIncludingChildren)
SettingsUI-->>SettingsUI: local state updated
Note over NodeTools: Node inclusion check
NodeTools->>PluginStorage: read ignoreHiddenLayersIncludingChildren
PluginStorage-->>NodeTools: flag
alt flag == true
NodeTools->>NodeTools: traverse parent chain → if any.hidden => exclude node
else
NodeTools->>NodeTools: exclude node only if node.hidden === true
end
sequenceDiagram
autonumber
participant UI as Push UI
participant API as Tolgee API
Note over UI,API: Push flow with screenshot/tag updates and refined errors
UI->>API: import keys (translations) [try]
alt import success
UI->>API: add tags to keys [try/catch]
UI->>API: for each screenshot -> update bigMeta [try/catch per screenshot]
alt too_many_uploaded_images
API-->>UI: error "too_many_uploaded_images"
UI-->>UI: set message -> "Too many uploaded images. Disable update screenshots in settings."
else other error
UI-->>UI: set generic push error
end
UI-->>UI: increment screenshot progress
else import failure
API-->>UI: error (invalid_project_api_key / other)
UI-->>UI: show specific or generic "Cannot push translations." message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Files/areas to pay extra attention to:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
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 |
eweren
changed the title
![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
🧹 Nitpick comments (3)
src/types.ts (1)
117-120: Type addition LGTM; add a brief doc for semantics/defaults.The new flag fits the model. Consider a short JSDoc describing behavior and its relation to ignoreHiddenLayers (default false; only applied when ignoreHiddenLayers is true). Optionally, group related flags under a nested shape in a future breaking change.
src/main/utils/settingsTools.ts (1)
70-89: Propagates the new flag correctly to document data.Looks good. If you want a cross-document default, also persist to global settings; if not, keeping it document-scoped (as now) is fine. Minor: consider nullish-coalescing defaults in readers for clarity.
Also applies to: 90-108
src/ui/views/Settings/StringsSection.tsx (1)
121-125: Wiring looks good; consider auto-reset + test id.
- When turning off “hidden layers”, also clear the child flag to avoid stale config and simplify UX.
- Add a data-cy on the nested checkbox for E2E coverage.
Example tweaks:
function handleIgnoreHiddenLayersChange( event: TargetedEvent<HTMLInputElement, Event> ): void { setIgnoreHiddenLayers(event.currentTarget.checked); - setTolgeeConfig({ - ...tolgeeConfig, - ignoreHiddenLayers: event.currentTarget.checked, - }); + if (!event.currentTarget.checked) { + setIgnoreHiddenLayersIncludingChildren(false); + } + setTolgeeConfig({ + ...tolgeeConfig, + ignoreHiddenLayers: event.currentTarget.checked, + ...(event.currentTarget.checked ? {} : { ignoreHiddenLayersIncludingChildren: false }), + }); } @@ - <Checkbox + <Checkbox + data-cy="settings_checkbox_hidden_layers_include_children" value={ignoreHiddenLayersIncludingChildren} onChange={handleIgnoreHiddenLayersIncludingChildrenChange} >Also applies to: 185-193, 300-314
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/utils/nodeTools.ts(1 hunks)src/main/utils/settingsTools.ts(2 hunks)src/types.ts(1 hunks)src/ui/views/Pull/Pull.tsx(1 hunks)src/ui/views/Push/Push.tsx(0 hunks)src/ui/views/Settings/StringsSection.tsx(3 hunks)
💤 Files with no reviewable changes (1)
- src/ui/views/Push/Push.tsx
🔇 Additional comments (1)
src/ui/views/Pull/Pull.tsx (1)
54-60: Error handling is correct and consistent; no code changes needed.After tracing the error flow: the client (src/ui/client/client.ts, line 114) converts errors to strings before throwing, so Pull.tsx correctly expects string error codes. The error handling across Pull.tsx, Push.tsx, and useAllTranslations.ts is uniform—all treat errors as strings. The "too_many_uploaded_images" error code is properly defined in the API schema and handled appropriately.
Note: Whether the backend actually throws "too_many_uploaded_images" should be confirmed with your API team, but that's outside code review scope.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/ui/views/Push/Push.tsx(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cypress
🔇 Additional comments (5)
src/ui/views/Push/Push.tsx (5)
1-436: PR objectives don't match the code changes in this file.The PR is titled "fix darkmode colors and hidden layers" and the objectives from issue #83 mention fixing input placeholder visibility, help/info icon visibility, and tooltip text visibility in dark mode. The AI summary also mentions "dark-mode primary text CSS" and an "ignoreHiddenLayersIncludingChildren" setting.
However, this file only contains error handling improvements and screenshot count logic changes—no dark mode CSS, no hidden layers functionality, and no changes related to the stated objectives.
Either the dark mode and hidden layers changes are in other files not provided for review, or this PR contains unrelated changes that should be in a separate PR.
255-282: Excellent error handling improvement.Wrapping the tag operations in a try/catch block allows translations to be pushed successfully even if tagging fails. The error message clearly communicates this partial success to users, which significantly improves the user experience.
286-311: Good error handling with specific error cases.Wrapping each screenshot update in a try/catch block and providing specific handling for
too_many_uploaded_imagesimproves resilience and user feedback. The error messages clearly indicate that translations were still updated successfully.However, note that the
screenshotCountincrement at line 300 conflicts with its usage at line 404 (already flagged separately).
317-333: Improved error messages for better user guidance.The enhanced error handling provides specific, actionable messages for different error scenarios:
- Clear distinction between API key errors, image upload limits, and import errors
- Helpful guidance (e.g., "Disable update screenshots in settings")
- More accurate error message: "Cannot push translations" vs. the previous generic "Cannot get translation data"
These changes significantly improve the user experience during error scenarios.
222-234: Verify that empty translation strings are correctly handled as "NEW" instead of "OVERRIDE".The conditional on line 230 is actually justified and not redundant. While
changedKeysitems always have anoldValueobject, the extractedoldValue.translationstring can be empty ("") or undefined. Empty strings are falsy in JavaScript, so the condition correctly treats them as "NEW" rather than "OVERRIDE".However, this represents a change in behavior: previously, changed keys may have always used "OVERRIDE", but now empty-to-nonempty translation updates use "NEW". Confirm this aligns with the intended Tolgee API semantics (i.e., whether an empty existing translation should be treated as a non-existent translation when pushing updates).
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/ui/views/Index/ListItem.tsx (1)
126-136: Icon color application works; consider centralizing via a CSS class.Inline
style={{ color: "var(--figma-color-text-secondary)" }}is fine, but a named class (e.g.,.secondaryIcon) keeps dark/light token usage consistent across the app and eases future theme tweaks.- <InsertLink - width={16} - height={16} - style={{ - color: "var(--figma-color-text-secondary)", - }} - /> + <InsertLink width={16} height={16} className={styles.secondaryIcon} />And in Index.css (or a shared icon stylesheet):
.secondaryIcon { color: var(--figma-color-text-secondary); }src/ui/views/Push/Push.tsx (3)
259-286: Nice resilience: tagging wrapped in try/catch with user feedback.This isolates non-critical tag failures from the main push flow. Consider also logging the original error for diagnostics if not already captured by
useApiMutation.- } catch (e) { + } catch (e) { + console.warn("addTagsToKeys failed", e); setErrorMessage( `Error adding tags. ${e}. Translations were still updated.` );
399-409: Checkbox label wired to computed count: LGTM.Minor UX nit: consider proper pluralization (“1 screenshot” vs “N screenshots”).
- <Text>Upload {totalScreenshotCount} screenshot(s)</Text> + <Text> + Upload {totalScreenshotCount} {totalScreenshotCount === 1 ? "screenshot" : "screenshots"} + </Text>
47-47: ClarifyscreenshotCountsemantics.It now tracks processed screenshots (not total). Rename to
uploadedScreenshotCount(orupdatedScreenshotCount) to avoid confusion withtotalScreenshotCount.- const [screenshotCount, setScreenshotCount] = useState(0); + const [uploadedScreenshotCount, setUploadedScreenshotCount] = useState(0);And update references accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/ui/views/Index/ListItem.tsx(1 hunks)src/ui/views/Push/Push.tsx(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/ui/views/Index/ListItem.tsx (1)
src/ui/icons/SvgIcons.tsx (1)
InsertLink(32-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cypress
🔇 Additional comments (3)
src/ui/views/Push/Push.tsx (3)
104-107: Good fix: derive total screenshot count from changes.Memoizing
totalScreenshotCountfromchanges?.screenshots.lengthresolves the initial “0 screenshots” label issue and keeps UI reactive.
423-425: Finish/Push gating now correctly respects screenshot-only pushes.Condition reads clearly and prevents showing “Finish” when screenshots are pending. Nice.
325-333: Error codes confirmed correct.All three error strings (
invalid_project_api_key,too_many_uploaded_images,import_keys_error) are legitimate server response codes defined in the ErrorResponseTyped enum insrc/ui/client/apiSchema.generated.ts, confirming they properly align with whatuseApiMutationthrows. Usage is consistent across the codebase (Pull.tsx, Push.tsx, Settings.tsx, useAllTranslations.ts).
|
Is included in #86 |
1 participant
fixes #83 and #84
Summary by CodeRabbit
Style
New Features
Bug Fixes / UX