-
Notifications
You must be signed in to change notification settings - Fork 52
Refactor: Update txiki.js version and unify binary fetching #51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…F and Image components
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
6 issues found across 20 files
Prompt for AI agents (all 6 issues)
Understand the root cause of the following 6 issues and fix them.
<file name=".gitignore">
<violation number="1" location=".gitignore:11">
Remove the trailing space so the ignore pattern matches `compile_commands.json`.</violation>
</file>
<file name="test/image/1/index.jsx">
<violation number="1" location="test/image/1/index.jsx:9">
The new relative path points to test/image/1/test/image/1/1.png, but only test/image/1/1.png exists, so the image will fail to load at runtime.</violation>
</file>
<file name="src/render/react/utils/helpers.ts">
<violation number="1" location="src/render/react/utils/helpers.ts:18">
fetchBinary should surface HTTP failures before decoding the body so callers don’t process error pages as binary data.</violation>
</file>
<file name="src/render/native/components/line/line_wrap.cpp">
<violation number="1" location="src/render/native/components/line/line_wrap.cpp:17">
The guard still allows argc==1, but the code below reads argv[1], which is out of bounds when only one argument is passed. Tighten the check to avoid undefined behavior/crashes.</violation>
</file>
<file name="src/render/native/components/dropdownlist/dropdownlist_wrap.cpp">
<violation number="1" location="src/render/native/components/dropdownlist/dropdownlist_wrap.cpp:16">
This guard should ensure argc >= 2 before reading argv[1]; otherwise the native function reads beyond the QuickJS argument list when only one argument is supplied.</violation>
</file>
<file name="src/render/native/components/roller/roller_wrap.cpp">
<violation number="1" location="src/render/native/components/roller/roller_wrap.cpp:16">
Accessing argv[1] and argv[2] requires verifying argc >= 3; otherwise calls with fewer arguments read beyond the QuickJS argv array and can crash native code.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 1 file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 1 file
|
I have fixed all the issues. Please review @derekstavis, thanks! |
|
Hi! Could someone review my PR? This repo needs several improvements before it's production-ready: fixing memory leaks and compilation warnings, improving runtime efficiency and render speed, refactoring the architecture, adding device benchmarks, etc. Our team wants to use this in production and I would like to help improve it. @derekstavis @kisvegabor |
…y point - Introduced option to build lvgljs as a static library. - Refactored engine initialization and execution into separate functions. - Updated CMakeLists.txt and Makefile to accommodate new build configurations. - Added main.cpp as the entry point for executable builds.
…esolution options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="CMakeLists.txt">
<violation number="1" location="CMakeLists.txt:11">
P2: LV_CONF_PATH is cached and can become stale if LVGLJS_CONF_DIR is changed, leaving the build using the wrong lv_conf.h.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| set(LVGLJS_CONF_DIR ${CMAKE_CURRENT_SOURCE_DIR}/deps CACHE INTERNAL "LVGLJS_CONF_DIR") | ||
| endif() | ||
|
|
||
| set(LV_CONF_PATH ${LVGLJS_CONF_DIR}/lv_conf.h CACHE INTERNAL "LV_CONF_PATH") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: LV_CONF_PATH is cached and can become stale if LVGLJS_CONF_DIR is changed, leaving the build using the wrong lv_conf.h.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At CMakeLists.txt, line 11:
<comment>LV_CONF_PATH is cached and can become stale if LVGLJS_CONF_DIR is changed, leaving the build using the wrong lv_conf.h.</comment>
<file context>
@@ -3,7 +3,12 @@ cmake_minimum_required(VERSION 3.16)
+ set(LVGLJS_CONF_DIR ${CMAKE_CURRENT_SOURCE_DIR}/deps CACHE INTERNAL "LVGLJS_CONF_DIR")
+endif()
+
+set(LV_CONF_PATH ${LVGLJS_CONF_DIR}/lv_conf.h CACHE INTERNAL "LV_CONF_PATH")
set(BUILD_NATIVE OFF CACHE BOOL "Build wasm3 with native support")
</file context>
- Add GroupManager singleton to manage lv_group_t instances - Modify BasicComponent appendChild/removeChild to handle group membership - Add unregisterContainer call in BasicComponent destructor - Remove auto lv_group_add_obj from all component constructors (21 files) - Add setInGroup method to View component for JS binding - Add DefaultGroup constant in React layer - Add group prop support in View React component - Add onFocusedStyle support in CommonComponentApi - Add test case in test/group/1/index.jsx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 issues found across 31 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/render/react/components/View/comp.ts">
<violation number="1" location="src/render/react/components/View/comp.ts:21">
P2: Group membership cannot be unset: once set it remains registered even if the group prop is removed or changed, leading to stale native state.</violation>
</file>
<file name="src/render/native/core/group/group_manager.cpp">
<violation number="1" location="src/render/native/core/group/group_manager.cpp:46">
P2: Auto-edit mode isn’t reset when the group becomes empty (or holds a non-editable single object), so editing can stay true after removals, leaving the group in a stale editing state with no focused object.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| function setViewProps(comp, newProps: ViewProps, oldProps: ViewProps) { | ||
| const setter = { | ||
| ...CommonComponentApi({ compName: "View", comp, newProps, oldProps }), | ||
| group(g) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Group membership cannot be unset: once set it remains registered even if the group prop is removed or changed, leading to stale native state.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/render/react/components/View/comp.ts, line 21:
<comment>Group membership cannot be unset: once set it remains registered even if the group prop is removed or changed, leading to stale native state.</comment>
<file context>
@@ -6,15 +6,23 @@ import {
function setViewProps(comp, newProps: ViewProps, oldProps: ViewProps) {
const setter = {
...CommonComponentApi({ compName: "View", comp, newProps, oldProps }),
+ group(g) {
+ if (g === DefaultGroup) {
+ comp.setInGroup(true);
</file context>
✅ Addressed in 90e252a
| if (!group) return; | ||
|
|
||
| uint32_t count = lv_group_get_obj_count(group); | ||
| if (count == 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Auto-edit mode isn’t reset when the group becomes empty (or holds a non-editable single object), so editing can stay true after removals, leaving the group in a stale editing state with no focused object.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/render/native/core/group/group_manager.cpp, line 46:
<comment>Auto-edit mode isn’t reset when the group becomes empty (or holds a non-editable single object), so editing can stay true after removals, leaving the group in a stale editing state with no focused object.</comment>
<file context>
@@ -0,0 +1,55 @@
+ if (!group) return;
+
+ uint32_t count = lv_group_get_obj_count(group);
+ if (count == 1) {
+ lv_obj_t* focused = lv_group_get_focused(group);
+ if (focused && lv_obj_is_editable(focused)) {
</file context>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="demo/viewpager/index.tsx">
<violation number="1" location="demo/viewpager/index.tsx:42">
P2: Active page state never updates when the pager is scrolled directly, so the dots and prev/next logic desync from the visible page</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| <View style={style.root}> | ||
| <View | ||
| ref={pagerRef} | ||
| style={style.pager} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Active page state never updates when the pager is scrolled directly, so the dots and prev/next logic desync from the visible page
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At demo/viewpager/index.tsx, line 42:
<comment>Active page state never updates when the pager is scrolled directly, so the dots and prev/next logic desync from the visible page</comment>
<file context>
@@ -0,0 +1,180 @@
+ <View style={style.root}>
+ <View
+ ref={pagerRef}
+ style={style.pager}
+ >
+ {Array.from({ length: pageCount }).map((_, i) => (
</file context>
- Introduced AddChildToDefGroup and AddCurToDefGroup symbols for group type management. - Updated View component to support groupType prop for specifying group behavior. - Modified GroupManager to handle new group types and improve child management. - Refactored BasicComponent methods to integrate with the new group system. - Updated documentation and examples to reflect changes in group handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 13 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/render/react/core/group/index.ts">
<violation number="1" location="src/render/react/core/group/index.ts:4">
P2: `AddToDefGroupType` collapses to `symbol` because `Symbol.for` returns plain `symbol`, so the intended two-token union is not enforced and any symbol type is accepted.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| export const AddChildToDefGroup = Symbol.for('AddChildToDefGroup'); | ||
| export const AddCurToDefGroup = Symbol.for('AddCurToDefGroup'); | ||
|
|
||
| export type AddToDefGroupType = typeof AddChildToDefGroup | typeof AddCurToDefGroup; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: AddToDefGroupType collapses to symbol because Symbol.for returns plain symbol, so the intended two-token union is not enforced and any symbol type is accepted.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/render/react/core/group/index.ts, line 4:
<comment>`AddToDefGroupType` collapses to `symbol` because `Symbol.for` returns plain `symbol`, so the intended two-token union is not enforced and any symbol type is accepted.</comment>
<file context>
@@ -1,2 +1,11 @@
+export const AddChildToDefGroup = Symbol.for('AddChildToDefGroup');
+export const AddCurToDefGroup = Symbol.for('AddCurToDefGroup');
+
+export type AddToDefGroupType = typeof AddChildToDefGroup | typeof AddCurToDefGroup;
+
+export function toNativeInGroupType(g?: AddToDefGroupType): 0 | 1 | undefined {
</file context>
Dependencies
Response.blob()support and fix compile error quickjs-ng/quickjs#1031)Changes
Unify image fetching with blob support
blob().arrayBuffer()pattern (requires txiki.js blob support)fetchBinary()utility inhelpers.tsCode cleanup
Buffer.from()conversionsgetImageBinary→fetchBinaryfor better clarityFix JS_IsArray API usage
JS_IsArray(ctx, obj)→JS_IsArray(obj)to match QuickJS APISummary by cubic
Updated txiki.js to enable blob-based fetches and unified binary loading for Image, GIF, and style assets. Adds opt-in LVGL focus navigation via default groups and a static library build option with configurable display resolution and LVGL config directory.
Dependencies
Refactors
Written for commit 90e252a. Summary will update on new commits.