Proof of concept: stlye tweking as a theme#156
Open
asturur wants to merge 2 commits into
Open
Conversation
📝 Walkthrough<review_stack_artifact> </review_stack_artifact> 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 4
🧹 Nitpick comments (1)
rust/frontend/src/models/settings.rs (1)
469-542: ⚡ Quick winAdd unit coverage for the new theme helpers.
discover_themes,normalize_theme, and the path-to-URL helpers now define persisted theme selection and external theme loading, but the test module still only covers the older settings helpers. A few table-driven cases for unknown themes, whitespace trimming, and paths with spaces would make this much safer to evolve.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/frontend/src/models/settings.rs` around lines 469 - 542, Add unit tests covering discover_themes, normalize_theme, path_to_file_url, and path_to_directory_url: write table-driven tests for normalize_theme to verify trimming, empty/unknown input returns DEFAULT_THEME and known names return the same string; test discover_themes by creating a temp themes_dir with .json files (including names with spaces), non-json files, and duplicate names to ensure external themes are discovered, deduplicated relative to BUILTIN_THEMES, sorted, and skipped when is_mister is true; test path_to_file_url encodes spaces and non-ASCII/unsafe bytes as %XX and path_to_directory_url appends a trailing '/' only when missing. Use tempdir/temp file fixtures and assert exact string results and list membership to cover edge cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/frontend/src/models/settings.rs`:
- Around line 514-542: path_to_file_url and path_to_directory_url currently
build file:// URIs by manual percent-encoding which yields incorrect forms on
Windows (e.g. file://C%3A%5C...) — replace this logic in path_to_file_url with a
platform-aware conversion that: resolves an absolute Path, converts backslashes
to forward slashes on Windows, ensures the correct authority/triple-slash prefix
(file:/// for local absolute paths with drive letters), and percent-encodes only
reserved characters (use existing std::path or url crate helper if available, or
implement per-platform rules), then have path_to_directory_url call the
corrected path_to_file_url and append the trailing slash if missing; add unit
tests covering POSIX and Windows-style inputs (drive-letter + backslashes) to
validate produced URIs round-trip with Qt expectations.
In `@src/ui/screens/MediaListScreen.qml`:
- Around line 84-93: Several layout properties (e.g., _activeLabelHeight,
_activeLabelBottomMargin, _bottomStatusLeftMargin, _bottomStatusRightMargin,
_topStripHeight, _topStripSlotMargin, _defaultGridBottomMargin,
_effectiveGridBottomMargin and the other occurrences you noted at lines
~298-299, 312-315, 412-415, 429-431) are bound directly to
BrowseLayouts.numberValue()/arithmetic and can produce fractional pixels; wrap
the raw profile-driven numeric values and any arithmetic that ultimately drives
x/y/width/height/margins/borders/fonts with the appropriate Sizing helper (most
commonly Sizing.px(...), or Sizing.stroke/center/half where semantically
required) so the computed values are quantized to pixel-aligned integers before
binding (replace expressions like BrowseLayouts.numberValue(...) or arithmetic
on those calls with Sizing.px(BrowseLayouts.numberValue(...)) or
Sizing.px(<existing expression>)).
In `@src/ui/screens/SystemsScreen.qml`:
- Around line 42-48: The profile-driven numeric properties (_activeLabelHeight,
_activeLabelBottomMargin, _bottomStatusLeftMargin, _bottomStatusRightMargin and
any other BrowseLayouts.numberValue results) must be converted to integer pixels
before driving geometry: wrap each BrowseLayouts.numberValue(...) with
Sizing.px(...), and likewise ensure any arithmetic that computes geometry (e.g.
_gridBottomMargin and _listOverlayBottomMargin which add/mix Sizing.pctH and
profile values) uses Sizing.px(...) on the profile parts (or the full
expression) so final width/height/margin values are integer-pixel (use Sizing.px
for values that feed x/y/width/height/border/margin/font sizes).
In `@src/ui/theme/BrowseLayouts.qml`:
- Around line 94-113: The code marks seen refs globally by setting
seenRefs[refPath] = true in BrowseLayouts._resolve and never clearing it,
causing false circular-reference detections across sibling recursion branches;
update the logic so each recursive call gets its own scoped tracking (e.g.,
clone or shallow-copy seenRefs for the branch before recursing) when calling
BrowseLayouts._resolve and when doing BrowseLayouts._lookup(refPath) so that
seenRefs is only shared down a single recursion path for functions handling
"ref:" (refPath) and not reused across unrelated sibling evaluations.
---
Nitpick comments:
In `@rust/frontend/src/models/settings.rs`:
- Around line 469-542: Add unit tests covering discover_themes, normalize_theme,
path_to_file_url, and path_to_directory_url: write table-driven tests for
normalize_theme to verify trimming, empty/unknown input returns DEFAULT_THEME
and known names return the same string; test discover_themes by creating a temp
themes_dir with .json files (including names with spaces), non-json files, and
duplicate names to ensure external themes are discovered, deduplicated relative
to BUILTIN_THEMES, sorted, and skipped when is_mister is true; test
path_to_file_url encodes spaces and non-ASCII/unsafe bytes as %XX and
path_to_directory_url appends a trailing '/' only when missing. Use tempdir/temp
file fixtures and assert exact string results and list membership to cover edge
cases.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 26c80f42-8c8d-4f1c-8fb8-304eb7ffb83b
📒 Files selected for processing (22)
rust/frontend/src/models/settings.rsrust/zaparoo-core/src/config.rsrust/zaparoo-core/src/persist.rsrust/zaparoo-core/src/platform_paths.rssrc/ui/app/Main.qmlsrc/ui/app/MainLayout.qmlsrc/ui/components/BrowseDetailPane.qmlsrc/ui/components/BrowseList.qmlsrc/ui/components/BrowseListDetailView.qmlsrc/ui/components/HeaderBar.qmlsrc/ui/components/PagedGrid.qmlsrc/ui/components/Tile.qmlsrc/ui/screens/GamesScreen.qmlsrc/ui/screens/MediaListScreen.qmlsrc/ui/screens/SettingsScreen.qmlsrc/ui/screens/SystemsScreen.qmlsrc/ui/theme/BrowseLayouts.qmlsrc/ui/theme/CMakeLists.txtsrc/ui/theme/Theme.qmlsrc/ui/theme/ThemePalette.qmlsrc/ui/theme/browse-themes/crt.jsonsrc/ui/theme/browse-themes/default.json
Comment on lines
+514
to
+542
| fn path_to_directory_url(path: &Path) -> String { | ||
| let mut url = path_to_file_url(path); | ||
| if !url.ends_with('/') { | ||
| url.push('/'); | ||
| } | ||
| url | ||
| } | ||
|
|
||
| fn path_to_file_url(path: &Path) -> String { | ||
| let absolute = if path.is_absolute() { | ||
| path.to_path_buf() | ||
| } else { | ||
| std::env::current_dir() | ||
| .unwrap_or_else(|_| ".".into()) | ||
| .join(path) | ||
| }; | ||
| let path_text = absolute.to_string_lossy(); | ||
| let mut encoded = String::with_capacity(path_text.len() + 8); | ||
| for byte in path_text.as_bytes() { | ||
| match byte { | ||
| b'A'..=b'Z' | b'a'..=b'z' | b'0'..=b'9' | b'-' | b'_' | b'.' | b'~' | b'/' => { | ||
| encoded.push(char::from(*byte)); | ||
| } | ||
| _ => { | ||
| let _ = std::fmt::Write::write_fmt(&mut encoded, format_args!("%{:02X}", byte)); | ||
| } | ||
| } | ||
| } | ||
| format!("file://{encoded}") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
from pathlib import PureWindowsPath
def current_encoder(path_text: str) -> str:
encoded = []
for b in path_text.encode():
if (48 <= b <= 57) or (65 <= b <= 90) or (97 <= b <= 122) or b in b"-_.~/":
encoded.append(chr(b))
else:
encoded.append(f"%{b:02X}")
return "file://" + "".join(encoded)
sample = r"C:\themes\crt theme"
print("current :", current_encoder(sample))
print("expected:", PureWindowsPath(sample).as_uri())
PYRepository: ZaparooProject/zaparoo-frontend
Length of output: 160
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "fn path_to_directory_url|fn path_to_file_url" rust/frontend/src/models/settings.rs
python - <<'PY'
import pathlib, re
p = pathlib.Path("rust/frontend/src/models/settings.rs")
text = p.read_text(encoding="utf-8")
m = re.search(r"fn path_to_file_url\\([^\\)]*\\) -> String\\s*\\{([\\s\\S]*?)\\n\\}", text)
print("found path_to_file_url:", bool(m))
PY
sed -n '500,560p' rust/frontend/src/models/settings.rsRepository: ZaparooProject/zaparoo-frontend
Length of output: 1802
Use a platform-aware file:// URL builder for themes_directory_url.
path_to_file_url() manually percent-encodes the absolute path and returns file://{encoded}; on Windows this produces an incorrect local-file URI shape (e.g., file://C%3A%5C... instead of file:///C:/...), so Qt round-tripping will fail. Replace this with a dedicated “file path → local file URI” helper and add tests for Windows paths (drive letters/backslashes). [lines 514-542]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/frontend/src/models/settings.rs` around lines 514 - 542,
path_to_file_url and path_to_directory_url currently build file:// URIs by
manual percent-encoding which yields incorrect forms on Windows (e.g.
file://C%3A%5C...) — replace this logic in path_to_file_url with a
platform-aware conversion that: resolves an absolute Path, converts backslashes
to forward slashes on Windows, ensures the correct authority/triple-slash prefix
(file:/// for local absolute paths with drive letters), and percent-encodes only
reserved characters (use existing std::path or url crate helper if available, or
implement per-platform rules), then have path_to_directory_url call the
corrected path_to_file_url and append the trailing slash if missing; add unit
tests covering POSIX and Windows-style inputs (drive-letter + backslashes) to
validate produced URIs round-trip with Qt expectations.
Comment on lines
+84
to
+93
| readonly property int _activeLabelHeight: BrowseLayouts.numberValue(root._gridProfile, "footer.activeLabelHeight", Sizing.pctH(7)) | ||
| readonly property int _activeLabelBottomMargin: BrowseLayouts.numberValue(root._gridProfile, "footer.activeLabelBottomMargin", 0) | ||
| readonly property bool _bottomStatusVisible: BrowseLayouts.boolValue(root._gridProfile, "footer.bottomStatusVisible", false) | ||
| readonly property int _bottomStatusLeftMargin: BrowseLayouts.numberValue(root._gridProfile, "footer.bottomStatusLeftMargin", 0) | ||
| readonly property int _bottomStatusRightMargin: BrowseLayouts.numberValue(root._gridProfile, "footer.bottomStatusRightMargin", 0) | ||
| readonly property bool _topStripVisible: BrowseLayouts.boolValue(root._viewProfile, "status.topStripVisible", true) | ||
| readonly property int _topStripHeight: BrowseLayouts.numberValue(root._viewProfile, "status.stripHeight", Sizing.pctH(7)) | ||
| readonly property int _topStripSlotMargin: BrowseLayouts.numberValue(root._viewProfile, "status.slotMargin", Sizing.pctW(5)) | ||
| readonly property int _defaultGridBottomMargin: root._bottomStatusVisible ? root._activeLabelBottomMargin + root._activeLabelHeight : Sizing.pctH(6) + root._activeLabelBottomMargin + root._activeLabelHeight | ||
| readonly property int _effectiveGridBottomMargin: root.gridBottomMargin > 0 ? root.gridBottomMargin : root._defaultGridBottomMargin |
There was a problem hiding this comment.
Quantize profile-driven geometry before binding to layout properties.
Several changed margins/heights/width expressions use raw BrowseLayouts.numberValue(...) (or arithmetic on it) directly. This can introduce fractional geometry and blur at 240p.
Proposed fix
- readonly property int _activeLabelHeight: BrowseLayouts.numberValue(root._gridProfile, "footer.activeLabelHeight", Sizing.pctH(7))
- readonly property int _activeLabelBottomMargin: BrowseLayouts.numberValue(root._gridProfile, "footer.activeLabelBottomMargin", 0)
+ readonly property int _activeLabelHeight: Sizing.px(BrowseLayouts.numberValue(root._gridProfile, "footer.activeLabelHeight", Sizing.pctH(7)))
+ readonly property int _activeLabelBottomMargin: Sizing.px(BrowseLayouts.numberValue(root._gridProfile, "footer.activeLabelBottomMargin", 0))
- readonly property int _bottomStatusLeftMargin: BrowseLayouts.numberValue(root._gridProfile, "footer.bottomStatusLeftMargin", 0)
- readonly property int _bottomStatusRightMargin: BrowseLayouts.numberValue(root._gridProfile, "footer.bottomStatusRightMargin", 0)
+ readonly property int _bottomStatusLeftMargin: Sizing.px(BrowseLayouts.numberValue(root._gridProfile, "footer.bottomStatusLeftMargin", 0))
+ readonly property int _bottomStatusRightMargin: Sizing.px(BrowseLayouts.numberValue(root._gridProfile, "footer.bottomStatusRightMargin", 0))
- readonly property int _topStripHeight: BrowseLayouts.numberValue(root._viewProfile, "status.stripHeight", Sizing.pctH(7))
- readonly property int _topStripSlotMargin: BrowseLayouts.numberValue(root._viewProfile, "status.slotMargin", Sizing.pctW(5))
+ readonly property int _topStripHeight: Sizing.px(BrowseLayouts.numberValue(root._viewProfile, "status.stripHeight", Sizing.pctH(7)))
+ readonly property int _topStripSlotMargin: Sizing.px(BrowseLayouts.numberValue(root._viewProfile, "status.slotMargin", Sizing.pctW(5)))
- anchors.leftMargin: BrowseLayouts.numberValue(root._listProfile, "list.cardSideMargin", Sizing.pctW(5))
- anchors.rightMargin: BrowseLayouts.numberValue(root._listProfile, "list.cardSideMargin", Sizing.pctW(5))
+ anchors.leftMargin: Sizing.px(BrowseLayouts.numberValue(root._listProfile, "list.cardSideMargin", Sizing.pctW(5)))
+ anchors.rightMargin: Sizing.px(BrowseLayouts.numberValue(root._listProfile, "list.cardSideMargin", Sizing.pctW(5)))
- width: Sizing.px(parent.width / 3) - root._bottomStatusLeftMargin
+ width: Sizing.px(parent.width / 3 - root._bottomStatusLeftMargin)
- width: Sizing.px(parent.width / 3) - root._bottomStatusRightMargin
+ width: Sizing.px(parent.width / 3 - root._bottomStatusRightMargin)As per coding guidelines, "Any value that drives x/y/width/height, border widths, margins, or font sizes must go through Sizing.px(), Sizing.stroke(), Sizing.center(), or Sizing.half()" and "fractional geometry is a bug everywhere."
Also applies to: 298-299, 312-315, 412-415, 429-431
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ui/screens/MediaListScreen.qml` around lines 84 - 93, Several layout
properties (e.g., _activeLabelHeight, _activeLabelBottomMargin,
_bottomStatusLeftMargin, _bottomStatusRightMargin, _topStripHeight,
_topStripSlotMargin, _defaultGridBottomMargin, _effectiveGridBottomMargin and
the other occurrences you noted at lines ~298-299, 312-315, 412-415, 429-431)
are bound directly to BrowseLayouts.numberValue()/arithmetic and can produce
fractional pixels; wrap the raw profile-driven numeric values and any arithmetic
that ultimately drives x/y/width/height/margins/borders/fonts with the
appropriate Sizing helper (most commonly Sizing.px(...), or
Sizing.stroke/center/half where semantically required) so the computed values
are quantized to pixel-aligned integers before binding (replace expressions like
BrowseLayouts.numberValue(...) or arithmetic on those calls with
Sizing.px(BrowseLayouts.numberValue(...)) or Sizing.px(<existing expression>)).
Comment on lines
+42
to
+48
| readonly property int _activeLabelHeight: BrowseLayouts.numberValue(systems._gridProfile, "footer.activeLabelHeight", Sizing.pctH(7)) | ||
| readonly property int _activeLabelBottomMargin: BrowseLayouts.numberValue(systems._gridProfile, "footer.activeLabelBottomMargin", 0) | ||
| readonly property bool _bottomStatusVisible: BrowseLayouts.boolValue(systems._gridProfile, "footer.bottomStatusVisible", false) | ||
| readonly property int _bottomStatusLeftMargin: BrowseLayouts.numberValue(systems._gridProfile, "footer.bottomStatusLeftMargin", 0) | ||
| readonly property int _bottomStatusRightMargin: BrowseLayouts.numberValue(systems._gridProfile, "footer.bottomStatusRightMargin", 0) | ||
| readonly property int _gridBottomMargin: systems._bottomStatusVisible ? systems._activeLabelBottomMargin + systems._activeLabelHeight : Sizing.pctH(6) + systems._activeLabelBottomMargin + systems._activeLabelHeight | ||
| readonly property int _listOverlayBottomMargin: systems._gridBottomMargin |
There was a problem hiding this comment.
Apply Sizing.px() to profile-derived dimensions and final width arithmetic.
The new profile-driven margins/heights/widths are bound from raw numeric values. This risks half-pixel layout under 240p and violates the integer-geometry rule.
Proposed fix
- readonly property int _activeLabelHeight: BrowseLayouts.numberValue(systems._gridProfile, "footer.activeLabelHeight", Sizing.pctH(7))
- readonly property int _activeLabelBottomMargin: BrowseLayouts.numberValue(systems._gridProfile, "footer.activeLabelBottomMargin", 0)
+ readonly property int _activeLabelHeight: Sizing.px(BrowseLayouts.numberValue(systems._gridProfile, "footer.activeLabelHeight", Sizing.pctH(7)))
+ readonly property int _activeLabelBottomMargin: Sizing.px(BrowseLayouts.numberValue(systems._gridProfile, "footer.activeLabelBottomMargin", 0))
- readonly property int _bottomStatusLeftMargin: BrowseLayouts.numberValue(systems._gridProfile, "footer.bottomStatusLeftMargin", 0)
- readonly property int _bottomStatusRightMargin: BrowseLayouts.numberValue(systems._gridProfile, "footer.bottomStatusRightMargin", 0)
+ readonly property int _bottomStatusLeftMargin: Sizing.px(BrowseLayouts.numberValue(systems._gridProfile, "footer.bottomStatusLeftMargin", 0))
+ readonly property int _bottomStatusRightMargin: Sizing.px(BrowseLayouts.numberValue(systems._gridProfile, "footer.bottomStatusRightMargin", 0))
- height: BrowseLayouts.numberValue(systems._viewProfile, "status.stripHeight", Sizing.pctH(7))
- slotMargin: BrowseLayouts.numberValue(systems._viewProfile, "status.slotMargin", Sizing.pctW(5))
+ height: Sizing.px(BrowseLayouts.numberValue(systems._viewProfile, "status.stripHeight", Sizing.pctH(7)))
+ slotMargin: Sizing.px(BrowseLayouts.numberValue(systems._viewProfile, "status.slotMargin", Sizing.pctW(5)))
- anchors.leftMargin: BrowseLayouts.numberValue(systems._viewProfile, "list.cardSideMargin", Sizing.pctW(5))
- anchors.rightMargin: BrowseLayouts.numberValue(systems._viewProfile, "list.cardSideMargin", Sizing.pctW(5))
+ anchors.leftMargin: Sizing.px(BrowseLayouts.numberValue(systems._viewProfile, "list.cardSideMargin", Sizing.pctW(5)))
+ anchors.rightMargin: Sizing.px(BrowseLayouts.numberValue(systems._viewProfile, "list.cardSideMargin", Sizing.pctW(5)))
- width: Sizing.px(parent.width / 3) - systems._bottomStatusLeftMargin
+ width: Sizing.px(parent.width / 3 - systems._bottomStatusLeftMargin)
- width: Sizing.px(parent.width / 3) - systems._bottomStatusRightMargin
+ width: Sizing.px(parent.width / 3 - systems._bottomStatusRightMargin)As per coding guidelines, "Any value that drives x/y/width/height, border widths, margins, or font sizes must go through Sizing.px(), Sizing.stroke(), Sizing.center(), or Sizing.half()" and "fractional geometry is a bug everywhere."
Also applies to: 187-188, 202-205, 273-274, 282-285, 299-302
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ui/screens/SystemsScreen.qml` around lines 42 - 48, The profile-driven
numeric properties (_activeLabelHeight, _activeLabelBottomMargin,
_bottomStatusLeftMargin, _bottomStatusRightMargin and any other
BrowseLayouts.numberValue results) must be converted to integer pixels before
driving geometry: wrap each BrowseLayouts.numberValue(...) with Sizing.px(...),
and likewise ensure any arithmetic that computes geometry (e.g.
_gridBottomMargin and _listOverlayBottomMargin which add/mix Sizing.pctH and
profile values) uses Sizing.px(...) on the profile parts (or the full
expression) so final width/height/margin values are integer-pixel (use Sizing.px
for values that feed x/y/width/height/border/margin/font sizes).
Comment on lines
+94
to
+113
| if (value.startsWith("ref:")) { | ||
| const refPath = value.substring("ref:".length); | ||
| if (seenRefs[refPath] === true) { | ||
| console.warn("BrowseLayouts: circular ref '" + refPath + "'"); | ||
| return undefined; | ||
| } | ||
| seenRefs[refPath] = true; | ||
| return BrowseLayouts._resolve(profile, BrowseLayouts._lookup(profile, refPath), seenRefs); | ||
| } | ||
|
|
||
| const fnMatch = value.match(/^([a-z]+)\((.*)\)$/); | ||
| if (fnMatch !== null) { | ||
| const fnName = fnMatch[1]; | ||
| const args = BrowseLayouts._splitArgs(fnMatch[2]).map(arg => BrowseLayouts._resolve(profile, arg, seenRefs)); | ||
| if (args.length === 2 && fnName === "min") | ||
| return Math.min(args[0], args[1]); | ||
| if (args.length === 2 && fnName === "max") | ||
| return Math.max(args[0], args[1]); | ||
| if (fnName === "sum") | ||
| return args.reduce((total, arg) => total + arg, 0); |
There was a problem hiding this comment.
Avoid false circular-reference hits by scoping seenRefs per recursion branch.
seenRefs[refPath] is set but never cleared, so sibling expressions that reuse the same valid ref: path can be flagged as circular incorrectly.
Proposed fix
if (value.startsWith("ref:")) {
const refPath = value.substring("ref:".length);
if (seenRefs[refPath] === true) {
console.warn("BrowseLayouts: circular ref '" + refPath + "'");
return undefined;
}
seenRefs[refPath] = true;
- return BrowseLayouts._resolve(profile, BrowseLayouts._lookup(profile, refPath), seenRefs);
+ const resolvedRef = BrowseLayouts._resolve(profile, BrowseLayouts._lookup(profile, refPath), seenRefs);
+ delete seenRefs[refPath];
+ return resolvedRef;
}
const fnMatch = value.match(/^([a-z]+)\((.*)\)$/);
if (fnMatch !== null) {
const fnName = fnMatch[1];
- const args = BrowseLayouts._splitArgs(fnMatch[2]).map(arg => BrowseLayouts._resolve(profile, arg, seenRefs));
+ const args = BrowseLayouts
+ ._splitArgs(fnMatch[2])
+ .map(arg => BrowseLayouts._resolve(profile, arg, Object.assign({}, seenRefs)));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ui/theme/BrowseLayouts.qml` around lines 94 - 113, The code marks seen
refs globally by setting seenRefs[refPath] = true in BrowseLayouts._resolve and
never clearing it, causing false circular-reference detections across sibling
recursion branches; update the logic so each recursive call gets its own scoped
tracking (e.g., clone or shallow-copy seenRefs for the branch before recursing)
when calling BrowseLayouts._resolve and when doing
BrowseLayouts._lookup(refPath) so that seenRefs is only shared down a single
recursion path for functions handling "ref:" (refPath) and not reused across
unrelated sibling evaluations.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ui/app/MainLayout.qml`:
- Around line 201-247: Add bindings for the remaining mutable Theme properties
(Theme.scrim, Theme.textPrimary, Theme.textLabel) using ThemePalette.colorValue
with ThemePalette.currentThemeId and the corresponding JSON keys (e.g.,
"palette.scrim", "palette.textPrimary", "palette.textLabel"), following the same
pattern as the existing Binding blocks (target: Theme, property: "...", value:
ThemePalette.colorValue(...)) so the mutable fields use the theme JSON values;
use the same default hex fallbacks as appropriate.
In `@src/ui/theme/ThemePalette.qml`:
- Around line 51-54: The code currently sets ThemePalette._themeCache[themeId] =
{ url: url, data: null } immediately, which makes transient load/parse failures
permanent; instead only add the cache entry with parsed data on success and
ensure failed loads do not leave a sticky null—e.g., when loading/parsing
succeeds set ThemePalette._themeCache[themeId] = { url, data: parsedData }, and
on any error remove the cache entry (delete ThemePalette._themeCache[themeId])
or leave it undefined so future lookups will retry; update the logic around
ThemePalette._themeCache assignments (the blocks that set data: null at themeId
and the similar assignment at the later occurrence) to implement this change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c6e5bb2-df82-40ca-8ebf-9d67210ce51d
📒 Files selected for processing (3)
src/ui/app/MainLayout.qmlsrc/ui/theme/Theme.qmlsrc/ui/theme/ThemePalette.qml
Comment on lines
+201
to
+247
| Binding { | ||
| target: Theme | ||
| property: "bgDeep" | ||
| value: ThemePalette.colorValue(ThemePalette.currentThemeId, "palette.bgDeep", "#0f0f23") | ||
| } | ||
|
|
||
| Binding { | ||
| target: Theme | ||
| property: "bgPanel" | ||
| value: ThemePalette.colorValue(ThemePalette.currentThemeId, "palette.bgPanel", "#1a1a35") | ||
| } | ||
|
|
||
| Binding { | ||
| target: Theme | ||
| property: "bgBar" | ||
| value: ThemePalette.colorValue(ThemePalette.currentThemeId, "palette.bgBar", "#0a0a15") | ||
| } | ||
|
|
||
| Binding { | ||
| target: Theme | ||
| property: "surfaceCard" | ||
| value: ThemePalette.colorValue(ThemePalette.currentThemeId, "palette.surfaceCard", "#2a2a45") | ||
| } | ||
|
|
||
| Binding { | ||
| target: Theme | ||
| property: "selectionSurface" | ||
| value: ThemePalette.colorValue(ThemePalette.currentThemeId, "palette.selectionSurface", "#3a3a66") | ||
| } | ||
|
|
||
| Binding { | ||
| target: Theme | ||
| property: "borderSubtle" | ||
| value: ThemePalette.colorValue(ThemePalette.currentThemeId, "palette.borderSubtle", "#1a1a2e") | ||
| } | ||
|
|
||
| Binding { | ||
| target: Theme | ||
| property: "borderMid" | ||
| value: ThemePalette.colorValue(ThemePalette.currentThemeId, "palette.borderMid", "#404060") | ||
| } | ||
|
|
||
| Binding { | ||
| target: Theme | ||
| property: "accent" | ||
| value: ThemePalette.colorValue(ThemePalette.currentThemeId, "palette.accent", "#FFB347") | ||
| } |
There was a problem hiding this comment.
Bind the remaining mutable theme palette fields.
This block applies only part of the palette. Theme.scrim, Theme.textPrimary, and Theme.textLabel were made mutable but are not theme-bound here, so matching JSON entries are never used.
💡 Proposed fix
Binding {
target: Theme
property: "accent"
value: ThemePalette.colorValue(ThemePalette.currentThemeId, "palette.accent", "`#FFB347`")
}
+
+ Binding {
+ target: Theme
+ property: "scrim"
+ value: ThemePalette.colorValue(ThemePalette.currentThemeId, "palette.scrim", "`#cc000000`")
+ }
+
+ Binding {
+ target: Theme
+ property: "textPrimary"
+ value: ThemePalette.colorValue(ThemePalette.currentThemeId, "palette.textPrimary", "`#ffffff`")
+ }
+
+ Binding {
+ target: Theme
+ property: "textLabel"
+ value: ThemePalette.colorValue(ThemePalette.currentThemeId, "palette.textLabel", "`#888888`")
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ui/app/MainLayout.qml` around lines 201 - 247, Add bindings for the
remaining mutable Theme properties (Theme.scrim, Theme.textPrimary,
Theme.textLabel) using ThemePalette.colorValue with ThemePalette.currentThemeId
and the corresponding JSON keys (e.g., "palette.scrim", "palette.textPrimary",
"palette.textLabel"), following the same pattern as the existing Binding blocks
(target: Theme, property: "...", value: ThemePalette.colorValue(...)) so the
mutable fields use the theme JSON values; use the same default hex fallbacks as
appropriate.
Comment on lines
+51
to
+54
| ThemePalette._themeCache[themeId] = { | ||
| url: url, | ||
| data: null | ||
| }; |
There was a problem hiding this comment.
Avoid caching failed theme loads as sticky null.
On Line 51 and Line 65, caching data: null makes transient load/parse failures permanent for that URL during the session, so later lookups won’t retry.
💡 Proposed fix
if (req.status !== 0 && (req.status < 200 || req.status >= 300)) {
console.warn("ThemePalette: failed to load theme '" + themeId + "' from " + url + " (status " + req.status + ")");
- ThemePalette._themeCache[themeId] = {
- url: url,
- data: null
- };
+ delete ThemePalette._themeCache[themeId];
return null;
}
@@
} catch (err) {
console.warn("ThemePalette: invalid JSON in theme '" + themeId + "': " + err);
- ThemePalette._themeCache[themeId] = {
- url: url,
- data: null
- };
+ delete ThemePalette._themeCache[themeId];
+ return null;
}
- return ThemePalette._themeCache[themeId].data;
+ return ThemePalette._themeCache[themeId] ? ThemePalette._themeCache[themeId].data : null;Also applies to: 65-68, 71-71
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ui/theme/ThemePalette.qml` around lines 51 - 54, The code currently sets
ThemePalette._themeCache[themeId] = { url: url, data: null } immediately, which
makes transient load/parse failures permanent; instead only add the cache entry
with parsed data on success and ensure failed loads do not leave a sticky
null—e.g., when loading/parsing succeeds set ThemePalette._themeCache[themeId] =
{ url, data: parsedData }, and on any error remove the cache entry (delete
ThemePalette._themeCache[themeId]) or leave it undefined so future lookups will
retry; update the logic around ThemePalette._themeCache assignments (the blocks
that set data: null at themeId and the similar assignment at the later
occurrence) to implement this change.
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
Summary by CodeRabbit
New Features
Style