Skip to content

Conversation

@eweren
Copy link
Contributor

@eweren eweren commented Sep 5, 2025

Fixes #78 by correctly checking for empty values

Fixes #79 by removing old usages of useWindowSize so now the user and only the user has full control over the window size

Fixes #80 by making the large list of translations a virtual list and optimizing the resizing logic, to remove the flickering while resizing.

Summary by CodeRabbit

  • New Features

    • Virtualized rendering for long node lists for better performance and smoother scrolling.
  • Refactor

    • Removed legacy window-size handling across several UI screens.
    • Global resize now emits once at startup instead of on every update.
  • Behavior Change

    • Pull/translation logic now treats empty or missing translations as missing keys.
  • Bug Fixes

    • Safer editor initialization/cleanup and resilient preview generation to avoid runtime errors.
  • Style

    • Minor formatting cleanup in a resize control.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 5, 2025

Walkthrough

Removed local useWindowSize calls from multiple UI views/components; added virtualization to NodeList with scroll-driven rendering and new props; GlobalState now emits RESIZE only on mount; tightened translation guard in getPullChanges; added guards/try-catch in StringsEditor/StringsSection; minor formatting tweak in ResizeHandle.

Changes

Cohort / File(s) Change Summary
Remove window-size hook usage
src/ui/components/Dialog/Dialog.tsx, src/ui/views/Index/Index.tsx, src/ui/views/PageSetup/PageSetup.tsx, src/ui/views/Settings/Settings.tsx
Deleted imports and invocations of useWindowSize and related size constants; retained component logic and signatures.
NodeList virtualization
src/ui/components/NodeList/NodeList.tsx
Added optional virtualization (virtualized?, rowHeight?, minVisibleRows?), scrollTop/containerHeight state and effects, visible window/overscan computation, spacer + translated inner container, and fallback full render when item count ≤ minVisibleRows.
GlobalState: mount-time RESIZE emission
src/ui/state/GlobalState.ts
Replaced sizeStack-dependent effect with a mount-only effect that reads last size or DEFAULT_SIZE and emits RESIZE once; removed unmount cleanup emission.
Tools: pull changes guard tightened
src/tools/getPullChanges.ts
Changed guard from if (value) to if (value?.translation) so nodes without a truthy translation are treated as missing keys.
Strings editor/preview guards
src/ui/views/Settings/StringsEditor.tsx, src/ui/views/Settings/StringsSection.tsx
Added guard early return when editor container missing and conditional editor cleanup; wrapped preview generation in try/catch with logging and fallback.
Minor formatting change
src/ui/components/ResizeHandle/ResizeHandle.tsx
Inserted blank line in resize handler; no behavioral change.

Sequence Diagram(s)

sequenceDiagram
  participant User as User (scroll)
  participant Container as Scroll Container
  participant NodeList as NodeList
  participant Renderer as Row Renderer

  Note over User,Container: User scrolls list
  User->>Container: scroll event (scrollTop)
  Container->>NodeList: update scrollTop
  NodeList->>NodeList: compute containerHeight, visibleStart/End, startIndex/endIndex (with overscan)
  NodeList->>Renderer: render sliced visibleItems (inner container translated)
Loading
sequenceDiagram
  participant App as App Mount
  participant Global as GlobalState
  participant Window as Window

  App->>Global: mount
  Global->>Global: read last size from sizeStack or DEFAULT_SIZE
  Global->>Window: emit RESIZE(initialSize)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Assessment against linked issues

Objective Addressed Explanation
Prevent window size resetting unexpectedly (#79) The PR removes per-component size hooks and emits RESIZE once on mount, but it's unclear if other code still resets size; runtime verification required.
Fix lag/twitch when resizing with many selected keys (#80) NodeList virtualization reduces list rendering cost but does not directly change window-resize handling; impact on resize twitching is uncertain and needs profiling.
Show message when key exists but translation missing (#78) getPullChanges now treats falsy translations as missing, but no UI change was added to display a user-facing message when translation is absent.

Assessment against linked issues: Out-of-scope changes

Code Change Explanation
NodeList virtualization (src/ui/components/NodeList/NodeList.tsx) Performance-focused list virtualization; not directly related to window-size stability (#79/#80) or UI messaging for missing translations (#78).
Guard change to translation check (src/tools/getPullChanges.ts) Changes classification for pull-processing but does not implement any UI notification or replacement behavior required by #78.

Possibly related PRs

Suggested reviewers

  • stepan662

"I hop through diff with whiskers bright,
I stash the sizes, set the height,
I prune the list to only show,
What fits the view — I make it so. 🐇"


📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 1e01099 and 6b59428.

📒 Files selected for processing (1)
  • src/tools/getPullChanges.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/tools/getPullChanges.ts
⏰ 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
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore or @coderabbit ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@eweren eweren changed the title fix: Issue 79 unstable window size fix: 79, 80 unstable window size, window resizing causing flicker Sep 5, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (5)
src/ui/state/GlobalState.ts (1)

36-40: Optional: Guard against dev double-invocation.

If your dev setup uses StrictMode (double-invokes effects), guard with a ref to avoid duplicate RESIZEs.

Add import:

// at top
import { useEffect, useState, useRef } from "preact/hooks";

Change effect:

const emittedOnceRef = useRef(false);
useEffect(() => {
  if (emittedOnceRef.current) return;
  emittedOnceRef.current = true;
  const last = sizeStack[sizeStack.length - 1];
  if (last) emit<ResizeHandler>("RESIZE", last);
  // eslint-disable-next-line react-hooks/exhaustive-deps
}, []);
src/ui/components/NodeList/NodeList.tsx (4)

51-56: Prefer JSX onScroll over manual listeners

Simpler, avoids listener churn on ref changes, and keeps the handler in the same component tree. Current approach is fine, but onScroll is idiomatic.

Apply this diff:

-// Handle scroll for virtualization
-const handleScroll = (e: Event) => {
-  const target = e.target as HTMLDivElement;
-  setScrollTop(target.scrollTop);
-};
-
-useEffect(() => {
-  const container = containerRef.current;
-  if (container) {
-    container.addEventListener("scroll", handleScroll, { passive: true });
-    return () => container.removeEventListener("scroll", handleScroll);
-  }
-}, []);
+const handleScroll = (e: UIEvent) => {
+  const target = e.currentTarget as HTMLDivElement;
+  setScrollTop(target.scrollTop);
+};

And wire it on the container:

-  <div
+  <div
     className={styles.container}
     ref={containerRef}
     style={{
       overflow: "auto",
       position: "relative",
       height: `${containerHeight}px`,
     }}
+    onScroll={handleScroll}
   >

Also applies to: 57-64


41-47: Replace “20px padding” magic number with a named constant

Improves maintainability and themed overrides.

Apply this diff:

-const DEFAULT_ROW_HEIGHT = 60;
-const MIN_VISIBLE_ROWS = 8;
+const DEFAULT_ROW_HEIGHT = 60;
+const MIN_VISIBLE_ROWS = 8;
+const BOTTOM_PADDING_PX = 20;
@@
-  const availableHeight = window.innerHeight - rect.top - 20; // 20px padding
+  const availableHeight = window.innerHeight - rect.top - BOTTOM_PADDING_PX;

65-82: Avoid nested keyed elements; no need for extra wrapper per row

renderItem already returns a keyed element (Fragment/NodeRow). Wrapping again with a keyed div produces redundant keys and extra DOM.

Apply this diff:

-          {visibleItems.map((item) => (
-            <div
-              key={item.id}
-              style={{
-                width: "100%",
-              }}
-            >
-              {renderItem(item)}
-            </div>
-          ))}
+          {visibleItems.map(renderItem)}

If width: 100% is needed, move it into the row component’s CSS.

Also applies to: 129-138


121-127: Micro-perf: hint compositor

When using translateY for large lists, add willChange: "transform" to reduce jank on some systems.

Apply this diff:

         <div
           style={{
             transform: `translateY(${offsetY}px)`,
             position: "absolute",
             top: 0,
             left: 0,
             right: 0,
+            willChange: "transform",
           }}
         >
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between a00359a and f4a2da7.

📒 Files selected for processing (3)
  • src/ui/components/NodeList/NodeList.tsx (3 hunks)
  • src/ui/components/ResizeHandle/ResizeHandle.tsx (1 hunks)
  • src/ui/state/GlobalState.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • src/ui/components/ResizeHandle/ResizeHandle.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
src/ui/state/GlobalState.ts (3)
src/ui/state/sizes.ts (1)
  • DEFAULT_SIZE (2-2)
src/utilities.ts (1)
  • emit (8-18)
src/types.ts (1)
  • ResizeHandler (33-36)
src/ui/components/NodeList/NodeList.tsx (1)
src/ui/components/NodeList/NodeRow.tsx (1)
  • NodeRow (19-94)
⏰ 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 (2)
src/ui/state/GlobalState.ts (1)

36-40: Verified: no unexpected RESIZE emits and GlobalState is top-level
All remaining RESIZE calls are intentional (host controllers and the user-driven ResizeHandle), and GlobalState is instantiated once in Plugin.tsx outside the Router, so it won’t remount on language switches or dialog opens.

src/ui/components/NodeList/NodeList.tsx (1)

20-22: Ensure DEFAULT_ROW_HEIGHT aligns with the actual NodeRow height
NodeRow.css doesn’t define a fixed container height—its height is determined by 6 px vertical padding, a 2 px row gap, plus the rendered line-heights of the two grid rows. Please measure the rendered row height (e.g., via getBoundingClientRect or browser devtools) and update either DEFAULT_ROW_HEIGHT (currently 60) or add an explicit height/min-height in NodeRow.css to match.

Comment on lines +15 to +17
virtualized?: boolean;
rowHeight?: number;
minVisibleRows?: number;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Expose and honor the virtualized prop; default to safe behavior

virtualized is declared in Props but never used. Currently, virtualization auto-enables whenever items.length > minVisibleRows, which is a behavior change and risky given uncertain row heights.

Apply this diff to make virtualization opt‑in and gate the code paths:

 export function NodeList<T extends { id: string }>({
   items,
   actionCallback,
   keyComponent,
   nsComponent,
   compact,
   onClick,
   row,
-  rowHeight = DEFAULT_ROW_HEIGHT,
-  minVisibleRows = MIN_VISIBLE_ROWS,
+  virtualized = false,
+  rowHeight = DEFAULT_ROW_HEIGHT,
+  minVisibleRows = MIN_VISIBLE_ROWS,
 }: Props<T>) {
@@
-  // If we have few items, render normally
-  if (items.length <= minVisibleRows) {
+  // Non-virtualized (default) or small lists → render normally
+  if (!virtualized || items.length <= minVisibleRows) {
     return (
       <div className={styles.container} ref={containerRef}>
         {items?.map(renderItem)}
       </div>
     );
   }

Also applies to: 31-33, 84-91

🤖 Prompt for AI Agents
In src/ui/components/NodeList/NodeList.tsx around lines 15-17 (and also update
the related logic at 31-33 and 84-91), the virtualized prop is declared but
never used so virtualization is automatically enabled based on items.length;
change this to make virtualization opt-in by default: set virtualized default to
false when destructuring props, ensure rowHeight and minVisibleRows keep
sensible defaults, and replace any checks like items.length > minVisibleRows
with a gate that requires virtualized === true (e.g., virtualized &&
items.length > minVisibleRows) so the virtualized code-paths (rendering, list
component selection, and any measurement logic) only run when the prop is
explicitly true.

Comment on lines +38 to +50
// Calculate container height based on available space
useEffect(() => {
if (containerRef.current) {
const rect = containerRef.current.getBoundingClientRect();
const availableHeight = window.innerHeight - rect.top - 20; // 20px padding
const calculatedHeight = Math.max(
minVisibleRows * rowHeight,
Math.min(availableHeight, items.length * rowHeight)
);
setContainerHeight(calculatedHeight);
}
}, [items.length, rowHeight, minVisibleRows]);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Recalculate height on user window resize

After the user resizes the plugin window, containerHeight stays stale (effect runs only on mount or when list/props change), undermining the PR goal. Recalculate on window resize; this reads size, it does not change it.

Apply this diff to reuse the same logic on resize:

-// Calculate container height based on available space
-useEffect(() => {
-  if (containerRef.current) {
-    const rect = containerRef.current.getBoundingClientRect();
-    const availableHeight = window.innerHeight - rect.top - 20; // 20px padding
-    const calculatedHeight = Math.max(
-      minVisibleRows * rowHeight,
-      Math.min(availableHeight, items.length * rowHeight)
-    );
-    setContainerHeight(calculatedHeight);
-  }
-}, [items.length, rowHeight, minVisibleRows]);
+// Calculate container height based on available space and keep it updated
+useEffect(() => {
+  const update = () => {
+    const el = containerRef.current;
+    if (!el) return;
+    const rect = el.getBoundingClientRect();
+    const availableHeight = window.innerHeight - rect.top - 20; // 20px padding
+    const calculatedHeight = Math.max(
+      minVisibleRows * rowHeight,
+      Math.min(availableHeight, items.length * rowHeight)
+    );
+    setContainerHeight(calculatedHeight);
+  };
+  update();
+  window.addEventListener("resize", update);
+  return () => window.removeEventListener("resize", update);
+}, [items.length, rowHeight, minVisibleRows]);
📝 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
// Calculate container height based on available space
useEffect(() => {
if (containerRef.current) {
const rect = containerRef.current.getBoundingClientRect();
const availableHeight = window.innerHeight - rect.top - 20; // 20px padding
const calculatedHeight = Math.max(
minVisibleRows * rowHeight,
Math.min(availableHeight, items.length * rowHeight)
);
setContainerHeight(calculatedHeight);
}
}, [items.length, rowHeight, minVisibleRows]);
// Calculate container height based on available space and keep it updated
useEffect(() => {
const update = () => {
const el = containerRef.current;
if (!el) return;
const rect = el.getBoundingClientRect();
const availableHeight = window.innerHeight - rect.top - 20; // 20px padding
const calculatedHeight = Math.max(
minVisibleRows * rowHeight,
Math.min(availableHeight, items.length * rowHeight)
);
setContainerHeight(calculatedHeight);
};
update();
window.addEventListener("resize", update);
return () => window.removeEventListener("resize", update);
}, [items.length, rowHeight, minVisibleRows]);
🤖 Prompt for AI Agents
In src/ui/components/NodeList/NodeList.tsx around lines 38 to 50, the effect
that computes containerHeight only runs on mount or when items/props change so
the height becomes stale after a user window resize; update the effect to also
subscribe to window resize events: extract the existing calculation into a
callback (or inline function) that reads containerRef and
recomputes/setContainerHeight, add a window.addEventListener('resize', handler)
when the effect runs and call the handler immediately to keep behavior the same,
and remove the listener in the effect cleanup with window.removeEventListener to
avoid leaks.

Comment on lines +93 to 107
// Virtualized rendering
const overscan = 3;
const visibleStart = Math.floor(scrollTop / rowHeight);
const visibleEnd = Math.min(
items.length - 1,
Math.ceil((scrollTop + containerHeight) / rowHeight)
);

const startIndex = Math.max(0, visibleStart - overscan);
const endIndex = Math.min(items.length - 1, visibleEnd + overscan);
const visibleItems = items.slice(startIndex, endIndex + 1);

const totalHeight = items.length * rowHeight;
const offsetY = startIndex * rowHeight;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fixed row height assumption is unsafe with NodeRow’s dynamic height

Virtualization here relies on a constant rowHeight, but NodeRow renders multi-line text (whiteSpace: "pre-wrap") and badges/tooltips, so rows can exceed 60px. This will cause visual overlap, clipping, or large gaps.

Options:

  • Short-term: keep virtualization off by default (see previous comment) until rows are made fixed-height in CSS to match rowHeight.
  • Medium-term: enforce a fixed height on NodeRow (and truncate/clamp text), and pass that value as rowHeight.
  • Long-term: switch to measurement-based virtualization (e.g., ResizeObserver-per-row or an existing lib supporting variable row heights).

Do you want me to draft the CSS constraints for NodeRow and wire rowHeight accordingly?

🤖 Prompt for AI Agents
In src/ui/components/NodeList/NodeList.tsx around lines 93-107, the
virtualization logic assumes a fixed rowHeight while NodeRow can grow past that
(multi-line text, badges, tooltips), causing overlap/gaps; short-term: disable
virtualization by default (render full list) until rows are constrained, and
medium-term: enforce a fixed height on NodeRow via CSS (clamp/truncate
multi-line text, ensure badges/tooltips don’t expand height) and then set
rowHeight to that exact pixel value; once fixed-height rows exist, re-enable
virtualization using the matching rowHeight or switch to a measurement-based
virtualization approach for variable heights.

Comment on lines +109 to +116
<div
className={styles.container}
ref={containerRef}
style={{
overflow: "auto",
position: "relative",
}}
>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Critical: containerHeight is computed but never applied → container won’t scroll; virtualization breaks

The virtualized container needs a fixed height; otherwise it expands to totalHeight and the scroll happens on the window, not on the container you’re listening to. This yields frozen rendering beyond the initial window.

Apply this diff to set the container height:

   return (
     <div
       className={styles.container}
       ref={containerRef}
       style={{
         overflow: "auto",
         position: "relative",
+        height: `${containerHeight}px`,
       }}
     >
📝 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
<div
className={styles.container}
ref={containerRef}
style={{
overflow: "auto",
position: "relative",
}}
>
<div
className={styles.container}
ref={containerRef}
style={{
overflow: "auto",
position: "relative",
height: `${containerHeight}px`,
}}
>
🤖 Prompt for AI Agents
In src/ui/components/NodeList/NodeList.tsx around lines 109 to 116, the
container div currently lacks an explicit height so the computed containerHeight
is never applied, causing the scroll to occur on the window and breaking
virtualization; update the inline style to include height: containerHeight (or
`${containerHeight}px`) so the container has a fixed height and scrolls
internally, ensuring virtualization works as intended.

Comment on lines +36 to +40
// Emit initial size on mount
useEffect(() => {
const size = sizeStack[sizeStack.length - 1] ?? DEFAULT_SIZE;
emit<ResizeHandler>("RESIZE", size);
return () => emit<ResizeHandler>("RESIZE", DEFAULT_SIZE);
}, [sizeStack]);
const initialSize = sizeStack[sizeStack.length - 1] ?? DEFAULT_SIZE;
emit<ResizeHandler>("RESIZE", initialSize);
}, []);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Do not emit DEFAULT_SIZE on mount; only emit when a prior user size exists.

Emitting DEFAULT_SIZE on every provider mount can still reset the window during remounts, contradicting the goal that only the user controls size. Emit only if we actually have a persisted size in sizeStack.

Apply this diff:

-    // Emit initial size on mount
-    useEffect(() => {
-      const initialSize = sizeStack[sizeStack.length - 1] ?? DEFAULT_SIZE;
-      emit<ResizeHandler>("RESIZE", initialSize);
-    }, []);
+    // Emit initial size on mount (only if we have a previously recorded size)
+    useEffect(() => {
+      const last = sizeStack[sizeStack.length - 1];
+      if (last) {
+        emit<ResizeHandler>("RESIZE", last);
+      }
+      // eslint-disable-next-line react-hooks/exhaustive-deps
+    }, []);
📝 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
// Emit initial size on mount
useEffect(() => {
const size = sizeStack[sizeStack.length - 1] ?? DEFAULT_SIZE;
emit<ResizeHandler>("RESIZE", size);
return () => emit<ResizeHandler>("RESIZE", DEFAULT_SIZE);
}, [sizeStack]);
const initialSize = sizeStack[sizeStack.length - 1] ?? DEFAULT_SIZE;
emit<ResizeHandler>("RESIZE", initialSize);
}, []);
// Emit initial size on mount (only if we have a previously recorded size)
useEffect(() => {
const last = sizeStack[sizeStack.length - 1];
if (last) {
emit<ResizeHandler>("RESIZE", last);
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);
🤖 Prompt for AI Agents
In src/ui/state/GlobalState.ts around lines 36 to 40, the current mount effect
emits DEFAULT_SIZE when sizeStack is empty; change it so the effect only emits a
resize event if there is an actual persisted user size in sizeStack.
Specifically, check that sizeStack.length > 0 before calling
emit<ResizeHandler>("RESIZE", ...), and only pass the last element of sizeStack
(no DEFAULT_SIZE fallback) so remounts don’t reset the window to the default.

@eweren eweren changed the title fix: 79, 80 unstable window size, window resizing causing flicker fix: 78, 79, 80 - disappearing texts, unstable window size, window resizing causing flicker Sep 5, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (3)
src/tools/getPullChanges.ts (3)

18-65: Confirm intent: empty-string translations are now treated as “missing”.

Using a truthiness check means "" is considered missing. If blank strings should be processed (not marked missing), switch to a nullish check.

-    if (value?.translation) {
+    if (value?.translation != null) { // allows empty string

If the current behavior (treating "" as missing) is intended, ignore this.


31-36: Avoid inserting an empty-string key into params when no plural parameter exists.

[tolgeeValue.parameter ?? ""] can produce a "" key. Prefer conditional spread to skip when the parameter is undefined.

-            params: {
-              ...node.paramsValues,
-              [tolgeeValue.parameter ?? ""]: node.pluralParamValue ?? "1",
-            },
+            params:
+              tolgeeValue.parameter
+                ? {
+                    ...node.paramsValues,
+                    [tolgeeValue.parameter]: node.pluralParamValue ?? "1",
+                  }
+                : node.paramsValues,

21-22: Normalize keyIsPlural to boolean to avoid undefined-vs-boolean mismatches.

If value.keyIsPlural can be undefined, comparisons/assignments may misfire. Coerce to boolean for comparison and when pushing nodes.

-        value.translation !== node.translation ||
-        value.keyIsPlural !== node.isPlural
+        value.translation !== node.translation ||
+        Boolean(value.keyIsPlural) !== node.isPlural
@@
-            isPlural: value.keyIsPlural,
+            isPlural: Boolean(value.keyIsPlural),
@@
-            isPlural: value.keyIsPlural,
+            isPlural: Boolean(value.keyIsPlural),
@@
-          isPlural: value.keyIsPlural,
+          isPlural: Boolean(value.keyIsPlural),

Also applies to: 39-41, 46-49, 58-61

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between f4a2da7 and 57a20b7.

📒 Files selected for processing (1)
  • src/tools/getPullChanges.ts (1 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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/ui/views/Settings/StringsSection.tsx (1)

63-67: Fix typos in user-facing help text

"the nearest" and "separators" are misspelled.

-        <li>section (name of thenearest section)</li>
-        <li>separtators can be “.” “:” “_” “-” etc.</li>
+        <li>section (name of the nearest section)</li>
+        <li>separators can be “.” “:” “_” “-” etc.</li>
🧹 Nitpick comments (3)
src/ui/views/Settings/StringsEditor.tsx (1)

161-162: Avoid early return that can permanently skip editor initialization

With an empty dependency array, returning early when ref.current is falsy means the effect never retries; if the ref were ever unset at the time the effect runs, the editor wouldn't mount. In practice Preact should have the ref set by effect time, but to be safe (and to reduce any flicker), prefer useLayoutEffect or ensure a retry path.

Apply this minimal change within the current effect:

-    if (!ref.current) return;
+    // Ref should be set after mount; using layout effect (see below) prevents race conditions.

Additionally, switch to layout effect and import it:

// imports
import { useLayoutEffect, useRef } from "preact/hooks";

// replace the useEffect([]) that mounts the editor with useLayoutEffect([])
useLayoutEffect(() => {
  // ...body unchanged (after removing the early return)...
}, []);
src/ui/views/Settings/StringsSection.tsx (2)

128-135: Harden preview against undefined casing to avoid unnecessary errors/logging

Pass a default when tolgeeConfig.variableCasing is undefined; this prevents avoidable exceptions that hit the catch.

-    try {
-      return getPreview(format, tolgeeConfig.variableCasing);
+    try {
+      return getPreview(format, tolgeeConfig.variableCasing ?? "");

242-246: Fix incorrect event/element cast on Casting to HTMLInputElement is incorrect for a <select>. Use currentTarget typed as HTMLSelectElement. - onChange={(e) => { - handleVariableCasingChange( - (e.target as HTMLInputElement).value - ); - }} + onChange={(e: TargetedEvent<HTMLSelectElement, Event>) => { + handleVariableCasingChange(e.currentTarget.value); + }} 📜 Review details Configuration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration: MCP integration is disabled by default for public repositories Jira integration is disabled by default for public repositories Linear integration is disabled by default for public repositories You can enable these sources in your CodeRabbit configuration. 📥 Commits Reviewing files that changed from the base of the PR and between 57a20b7 and 1e01099. 📒 Files selected for processing (2) src/ui/views/Settings/StringsEditor.tsx (2 hunks) src/ui/views/Settings/StringsSection.tsx (1 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 (1) src/ui/views/Settings/StringsEditor.tsx (1) 209-212: Cleanup safety — LGTM Conditional destroy + nulling the ref prevents double-destroy and avoids leaks.

@JanCizmar JanCizmar merged commit 1ee19b8 into tolgee:main Sep 8, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Window resize feature causes the window to lag and twitch Unstable window size Text disappears when there is a key but there is no translation

2 participants