feat(themes): Theme Manager Phase 1 — safe declarative themes + agent SDK + variant shell#513
Merged
Conversation
… state, revertible)
…n-interactive layer)
…restore active theme on boot
|
Warning Review limit reached
More reviews will be available in 2 minutes and 26 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughThis PR implements a complete, client-controlled theme system spanning backend validation, persistence, and API; frontend state management; visual effect components; settings UI; and persistent theme restoration on boot with accessibility safeguards. ChangesTheme System End-to-End
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 14
🧹 Nitpick comments (4)
desktop/src/components/dock/__tests__/DockVariants.test.tsx (1)
7-18: ⚡ Quick winReset mutated stores between test cases.
These tests write into singleton Zustand stores and never restore them, so later cases can inherit
pinnedorstructurestate from earlier ones.Proposed fix
-import { describe, it, expect } from "vitest"; +import { beforeEach, describe, it, expect } from "vitest"; @@ describe("Dock variant selection", () => { + beforeEach(() => { + useDockStore.setState({ pinned: [] } as never); + useThemeStore.setState({ structure: {} } as never); + }); + it("renders the macos-dock variant by default and shows pinned apps", () => { useDockStore.setState({ pinned: ["files"] } as never); useThemeStore.setState({ structure: {} } as never);🤖 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 `@desktop/src/components/dock/__tests__/DockVariants.test.tsx` around lines 7 - 18, The tests mutate singleton Zustand stores (useDockStore and useThemeStore) and don't reset them, causing cross-test pollution; add a beforeEach or afterEach hook in DockVariants.test.tsx that resets both stores to a known baseline (e.g., useDockStore.setState({ pinned: [] } as never) and useThemeStore.setState({ structure: {} } as never)) so each test (the ones that call useDockStore.setState and useThemeStore.setState and render <Dock />) starts with a clean store state.desktop/src/theme/effects/CrtEffect.tsx (1)
8-23: ⚡ Quick winRespect
prefers-reduced-motionfor the CRT flicker.This runs a perpetual flicker animation. Users with reduced-motion enabled should get the static vignette without the animation.
Suggested change
export function CrtEffect({ params: _params }: Props) { return ( <> <style>{` `@keyframes` crt-flicker { 0% { opacity: 0.92; } 50% { opacity: 1; } 100% { opacity: 0.92; } } + + `@media` (prefers-reduced-motion: reduce) { + .crt-effect-overlay { + animation: none !important; + } + } `}</style> <div + className="crt-effect-overlay" style={{ position: "absolute", inset: 0, pointerEvents: "none", background: "radial-gradient(ellipse at center, transparent 60%, rgba(0,0,0,0.35) 100%)", animation: "crt-flicker 0.15s infinite", }} />🤖 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 `@desktop/src/theme/effects/CrtEffect.tsx` around lines 8 - 23, In the CrtEffect component, the vignette/div currently always applies the "crt-flicker" animation; update the implementation to respect the user's prefers-reduced-motion setting by disabling the animation when reduced motion is requested: either add a CSS media rule (`@media` (prefers-reduced-motion: reduce) { .crt-flicker { animation: none !important; } }) to the injected <style> or detect window.matchMedia("(prefers-reduced-motion: reduce)") in the component and remove the animation property from the div's inline style, ensuring the element still renders the static radial-gradient vignette but without animation.desktop/src/theme/__tests__/cursor-effect.test.tsx (1)
10-20: ⚡ Quick winAdd a cleanup regression for cursor restoration.
These cases cover apply/fallback, but the critical contract here is restoring the previous body cursor on unmount.
Suggested test
describe("CursorEffect", () => { it("applies a safe cursor value", () => { render(<CursorEffect params={{ cursor: "pointer" }} />); expect(document.body.style.cursor).toBe("pointer"); }); it("ignores an unsafe cursor value and falls back to crosshair", () => { render(<CursorEffect params={{ cursor: "url(/x.png)" }} />); expect(document.body.style.cursor).toBe("crosshair"); }); + + it("restores the previous cursor on unmount", () => { + document.body.style.cursor = "wait"; + const { unmount } = render(<CursorEffect params={{ cursor: "pointer" }} />); + expect(document.body.style.cursor).toBe("pointer"); + unmount(); + expect(document.body.style.cursor).toBe("wait"); + }); });🤖 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 `@desktop/src/theme/__tests__/cursor-effect.test.tsx` around lines 10 - 20, The tests for CursorEffect currently verify applying safe and fallback cursor values but miss verifying that the previous document.body.style.cursor is restored on unmount; update the cursor-effect.test.tsx tests to capture the original body cursor, render CursorEffect (using render from testing-library), then unmount (or call cleanup) and assert document.body.style.cursor equals the saved original; reference the CursorEffect component and the existing test cases ("applies a safe cursor value", "ignores an unsafe cursor value and falls back to crosshair") and add the restore assertion after unmounting to ensure cleanup behavior is validated.desktop/src/apps/SettingsApp/__tests__/ThemesPanel.test.tsx (1)
12-29: ⚡ Quick winAdd a revert-from-existing-theme regression case.
This suite only verifies reverting back to an empty root style, so it won't catch the real user flow where a non-default theme is already active before opening the panel. A test that seeds an existing applied theme, previews another one, and then clicks Revert would protect the contract this UI is supposed to guarantee.
🤖 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 `@desktop/src/apps/SettingsApp/__tests__/ThemesPanel.test.tsx` around lines 12 - 29, The test suite misses a regression case where a non-default theme is already applied; add a new test in ThemesPanel.test.tsx that seeds an existing applied theme by setting document.documentElement.style (e.g., setProperty for "--color-accent" to a known value) before rendering <ThemesPanel />, then simulate selecting/previewing another theme (like "Ocean Blue"), click the "Revert" button and assert the original seeded CSS variable is restored (not cleared), and finally clean up the seeded style; reference the ThemesPanel component, the "Revert" button role/name and the CSS variable "--color-accent" when locating assertions.
🤖 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 `@desktop/src/apps/SettingsApp/ThemesPanel.tsx`:
- Around line 26-27: priorConfig is initialized to EMPTY_CONFIG and never
updated so the first "Revert" restores defaults; modify the preview flow in
ThemesPanel to snapshot the current theme from the store into priorConfig when a
preview is started (e.g., in the function that begins previews or the preview
button handler), and only clear or overwrite that snapshot when the preview is
committed or cancelled (e.g., in the commit/cancel handlers). Specifically, call
setPriorConfig(currentThemeFromStore) at preview start (use the same store
selector used elsewhere in ThemesPanel) and ensure commitPreview and
cancelPreview reset priorConfig appropriately so revert uses the original
snapshot.
In `@desktop/src/components/dock/MacosDock.tsx`:
- Around line 12-13: The dock builds runningNotPinned from windows.map(...)
which can include duplicate appIds when an app has multiple windows; change the
logic in MacosDock (and the analogous Windows dock file) to deduplicate app IDs
before filtering against pinned — e.g., compute a unique set from windows
(reference variables: windows, runningAppIds, runningNotPinned) then filter that
set by pinned to produce runningNotPinned so each running app appears only once.
In `@desktop/src/components/dock/WindowsTaskbar.tsx`:
- Around line 5-7: Replace the current duplicate-preserving runningAppIds logic
with a deduplicated list of appIds so each running app produces a single taskbar
item: create a Map or use Array.from(new Set(windows.map(w => w.appId))) to
derive unique runningAppIds, and update runningNotPinned/items to use that
deduplicated list; also ensure the click handler (onAppClick) receives or
resolves to a specific window id for that app (e.g., map appId -> first or last
window id from windows array or maintain a Map<string, string[]> of appId to
windowIds and pass a representative window id when invoking onAppClick) so each
taskbar item still targets a concrete window.
In `@desktop/src/components/SafetyFloor.tsx`:
- Around line 19-23: The fixed escape-hatch button in SafetyFloor is positioned
with hardcoded top/right values that can sit under notches; update the
container's inline style (the div containing the button in SafetyFloor) to
account for safe-area insets using CSS env()—for example set top and right using
calc with env(safe-area-inset-top, 0px) and env(safe-area-inset-right, 0px)
(e.g. top: `calc(env(safe-area-inset-top, 0px) + 4px)` and right:
`calc(env(safe-area-inset-right, 0px) + 8px)`), so the button (opened by
openPanel) stays visible on mobile devices with notches.
In `@desktop/src/stores/theme-store.ts`:
- Around line 129-139: applyThemeConfig currently updates tokens, structure, and
effects but never applies cfg.wallpaper; update applyThemeConfig (and the
similar flows around lines 173-198 and 204-219) to set and apply the wallpaper
as part of the same flow by reading cfg.wallpaper and invoking the existing
wallpaper application path (e.g., call the function that applies wallpapers or
update the wallpaper store state such as useWallpaperStore.setState({ active:
cfg.wallpaper }) or call applyWallpaper(cfg.wallpaper)), ensuring the DOM/active
wallpaper is updated after revertTheme and before returning so theme
preview/restore keeps wallpaper in sync.
In `@desktop/src/theme/effects/EffectsLayer.tsx`:
- Around line 17-19: The mapped effect elements in EffectsLayer.tsx use a
fragile index key (key={i}) which causes remounts on reorder; change the key to
a stable identifier for each effect instance (use e.module if each effect is
unique per theme, or use a persisted id field like e.id on the effect object) so
components from REGISTRY (e.g., CursorEffect) are not remounted and do not run
stale cleanup logic; update the map to use that stable key and ensure the effect
objects include the chosen identifier when created.
In `@tests/conftest.py`:
- Around line 133-136: The fixture creates and initializes the themes store
(themes.init()) but never closes it in the teardown; modify the fixture's
post-yield cleanup to check themes._db (or themes.is_initialized if available)
and call await themes.close() so the DB handle is closed after tests. Locate the
themes variable in the fixture (where themes = app.state.themes, themes.init(),
and the yield occurs) and add an await themes.close() guarded by the same
condition used during setup to avoid closing a non-initialized store.
In `@tests/themes/test_package.py`:
- Line 27: Split the single-line chained assignment into two statements so it no
longer triggers E702: create the dict from MANIFEST into the variable bad, then
on the next line assign bad["tokens"] = {"--evil": "x"}; update the statement
that currently reads as bad = dict(MANIFEST); bad["tokens"] = {"--evil": "x"} to
two separate lines referencing the variable bad and the MANIFEST source.
In `@tests/themes/test_schema.py`:
- Line 14: Split the chained statements into two separate statements to satisfy
lint rule E702: call _base() and assign it to cfg on its own line, then on the
next line perform the tokens assignment (e.g., replace "cfg = _base();
cfg[\"tokens\"] = {...}" with "cfg = _base()" followed by "cfg[\"tokens\"] =
{...}"). Apply the same change for every occurrence using _base() and cfg in
this file (lines where cfg = _base(); cfg["tokens"] = ...).
In `@tinyagentos/routes/themes.py`:
- Around line 45-47: Validate and sanitize theme_id before using it to build
root: compute base = _themes_root(request).resolve(), then ensure theme_id does
not contain path separators or traversal segments (reject values with "/" or "\"
or ".." or use only the final path component via Path(theme_id).name), then set
root = (base / theme_id / "assets").resolve() and assert
str(root).startswith(str(base) + "/") before resolving target; keep the existing
target check that target is a file and is under root. This prevents
attacker-controlled theme_id from escaping the intended themes directory while
preserving the path traversal check for path.
In `@tinyagentos/themes/package.py`:
- Around line 41-49: The extraction currently writes files directly into
theme_dir while iterating over zf.namelist(), leaving partial files if a later
member fails; change install logic to extract into a temporary directory inside
themes_root (e.g., create temp_dir next to theme_dir), perform the same
validation checks (member.endswith("/"), path-safety check against temp_dir,
dest.parent.mkdir, write_bytes) against that temp_dir, and only atomically
rename/move temp_dir to theme_dir after all members succeed; additionally ensure
any exception during extraction removes the temp_dir (or on failure fall back to
cleanup) and keep raising ThemePackageError for unsafe paths as before so
partial installs are never served.
- Around line 13-19: After loading with yaml.safe_load (variable data), validate
that data is a mapping/dict before accessing keys; if data is not a dict (e.g.,
list, str, int) raise a ThemePackageError indicating an invalid/malformed
manifest. Update the block around yaml.safe_load in package.py to check
isinstance(data, dict) (or collections.abc.Mapping) and raise ThemePackageError
with a clear message prior to the for key in _REQUIRED loop so subsequent
data.get(...) calls are safe.
In `@tinyagentos/themes/schema.py`:
- Around line 64-69: The loop over structure items currently assumes each
structural config (conf) is a mapping and calls conf.get("variant"), which
raises AttributeError for non-dict values (e.g., a string) and causes a 500;
update the validation inside the for surface, conf in structure.items() loop to
first ensure conf is either None or a dict/mapping before accessing conf.get —
if conf is of the wrong type, raise ThemeError with a clear message like
"invalid structural config for {surface}"; then continue to check variant
against _VARIANTS[surface] as before.
- Around line 89-94: The code currently builds requires via
set(cfg.get("requires", []) or []) which accepts strings (turning them into sets
of chars); change it to first fetch raw = cfg.get("requires", None), normalize
None to an empty list, and explicitly reject any non-list/tuple input by raising
a clear ValueError (or TypeError) indicating "requires must be a list". After
that, compute requires = set(raw) | _SAFETY_FLOOR and continue setting
out["requires"] = sorted(requires); reference the local names cfg, requires,
_SAFETY_FLOOR, and out to locate and update this validation.
---
Nitpick comments:
In `@desktop/src/apps/SettingsApp/__tests__/ThemesPanel.test.tsx`:
- Around line 12-29: The test suite misses a regression case where a non-default
theme is already applied; add a new test in ThemesPanel.test.tsx that seeds an
existing applied theme by setting document.documentElement.style (e.g.,
setProperty for "--color-accent" to a known value) before rendering <ThemesPanel
/>, then simulate selecting/previewing another theme (like "Ocean Blue"), click
the "Revert" button and assert the original seeded CSS variable is restored (not
cleared), and finally clean up the seeded style; reference the ThemesPanel
component, the "Revert" button role/name and the CSS variable "--color-accent"
when locating assertions.
In `@desktop/src/components/dock/__tests__/DockVariants.test.tsx`:
- Around line 7-18: The tests mutate singleton Zustand stores (useDockStore and
useThemeStore) and don't reset them, causing cross-test pollution; add a
beforeEach or afterEach hook in DockVariants.test.tsx that resets both stores to
a known baseline (e.g., useDockStore.setState({ pinned: [] } as never) and
useThemeStore.setState({ structure: {} } as never)) so each test (the ones that
call useDockStore.setState and useThemeStore.setState and render <Dock />)
starts with a clean store state.
In `@desktop/src/theme/__tests__/cursor-effect.test.tsx`:
- Around line 10-20: The tests for CursorEffect currently verify applying safe
and fallback cursor values but miss verifying that the previous
document.body.style.cursor is restored on unmount; update the
cursor-effect.test.tsx tests to capture the original body cursor, render
CursorEffect (using render from testing-library), then unmount (or call cleanup)
and assert document.body.style.cursor equals the saved original; reference the
CursorEffect component and the existing test cases ("applies a safe cursor
value", "ignores an unsafe cursor value and falls back to crosshair") and add
the restore assertion after unmounting to ensure cleanup behavior is validated.
In `@desktop/src/theme/effects/CrtEffect.tsx`:
- Around line 8-23: In the CrtEffect component, the vignette/div currently
always applies the "crt-flicker" animation; update the implementation to respect
the user's prefers-reduced-motion setting by disabling the animation when
reduced motion is requested: either add a CSS media rule (`@media`
(prefers-reduced-motion: reduce) { .crt-flicker { animation: none !important; }
}) to the injected <style> or detect window.matchMedia("(prefers-reduced-motion:
reduce)") in the component and remove the animation property from the div's
inline style, ensuring the element still renders the static radial-gradient
vignette but without animation.
🪄 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 Plus
Run ID: 9e246b50-ce2a-4fae-8fdf-78a09d949a67
📒 Files selected for processing (40)
desktop/src/App.tsxdesktop/src/apps/SettingsApp.tsxdesktop/src/apps/SettingsApp/ThemesPanel.tsxdesktop/src/apps/SettingsApp/__tests__/ThemesPanel.test.tsxdesktop/src/components/Dock.tsxdesktop/src/components/SafetyFloor.tsxdesktop/src/components/__tests__/SafetyFloor.test.tsxdesktop/src/components/dock/DockVariants.tsdesktop/src/components/dock/MacosDock.tsxdesktop/src/components/dock/WindowsTaskbar.tsxdesktop/src/components/dock/__tests__/DockVariants.test.tsxdesktop/src/stores/__tests__/restore-theme.test.tsdesktop/src/stores/__tests__/theme-apply.test.tsdesktop/src/stores/__tests__/wallpaper.test.tsdesktop/src/stores/theme-store.tsdesktop/src/theme/__tests__/builtin-themes.test.tsdesktop/src/theme/__tests__/cursor-effect.test.tsxdesktop/src/theme/__tests__/safety-regression.test.tsxdesktop/src/theme/builtin-themes.tsdesktop/src/theme/effects/CrtEffect.tsxdesktop/src/theme/effects/CursorEffect.tsxdesktop/src/theme/effects/EffectsLayer.tsxdesktop/src/theme/effects/GlowEffect.tsxdesktop/src/theme/effects/ScanlinesEffect.tsxdesktop/src/theme/effects/__tests__/EffectsLayer.test.tsxdesktop/src/theme/theme-config.tstests/conftest.pytests/themes/test_e2e.pytests/themes/test_mcp_tools.pytests/themes/test_package.pytests/themes/test_routes.pytests/themes/test_schema.pytests/themes/test_store.pytinyagentos/app.pytinyagentos/routes/mcp.pytinyagentos/routes/themes.pytinyagentos/themes/__init__.pytinyagentos/themes/package.pytinyagentos/themes/schema.pytinyagentos/themes/store.py
Comment on lines
+26
to
+27
| const [priorConfig, setPriorConfig] = useState<ThemeConfig>(EMPTY_CONFIG); | ||
|
|
There was a problem hiding this comment.
Snapshot the currently applied theme before the first preview.
priorConfig starts as EMPTY_CONFIG and never syncs to the active theme, so the first Revert in a Settings session restores an empty/default shell instead of the user's real pre-preview theme. Please capture the currently applied config from the store when previewing starts, and only keep that original snapshot until the preview is committed or cancelled.
Also applies to: 48-50
🤖 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 `@desktop/src/apps/SettingsApp/ThemesPanel.tsx` around lines 26 - 27,
priorConfig is initialized to EMPTY_CONFIG and never updated so the first
"Revert" restores defaults; modify the preview flow in ThemesPanel to snapshot
the current theme from the store into priorConfig when a preview is started
(e.g., in the function that begins previews or the preview button handler), and
only clear or overwrite that snapshot when the preview is committed or cancelled
(e.g., in the commit/cancel handlers). Specifically, call
setPriorConfig(currentThemeFromStore) at preview start (use the same store
selector used elsewhere in ThemesPanel) and ensure commitPreview and
cancelPreview reset priorConfig appropriately so revert uses the original
snapshot.
Comment on lines
+12
to
+13
| const runningAppIds = windows.map((w) => w.appId); | ||
| const runningNotPinned = runningAppIds.filter((id) => !pinned.includes(id)); |
There was a problem hiding this comment.
Deduplicate running app ids before rendering icons.
If an unpinned app has multiple open windows, runningNotPinned will contain that appId more than once, so the dock renders duplicate icons for the same app. The Windows variant has the same bug from the copied logic.
Proposed fix
- const runningAppIds = windows.map((w) => w.appId);
+ const runningAppIds = [...new Set(windows.map((w) => w.appId))];
const runningNotPinned = runningAppIds.filter((id) => !pinned.includes(id));📝 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
| const runningAppIds = windows.map((w) => w.appId); | |
| const runningNotPinned = runningAppIds.filter((id) => !pinned.includes(id)); | |
| const runningAppIds = [...new Set(windows.map((w) => w.appId))]; | |
| const runningNotPinned = runningAppIds.filter((id) => !pinned.includes(id)); |
🤖 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 `@desktop/src/components/dock/MacosDock.tsx` around lines 12 - 13, The dock
builds runningNotPinned from windows.map(...) which can include duplicate appIds
when an app has multiple windows; change the logic in MacosDock (and the
analogous Windows dock file) to deduplicate app IDs before filtering against
pinned — e.g., compute a unique set from windows (reference variables: windows,
runningAppIds, runningNotPinned) then filter that set by pinned to produce
runningNotPinned so each running app appears only once.
Comment on lines
+5
to
+7
| const runningAppIds = windows.map((w) => w.appId); | ||
| const runningNotPinned = runningAppIds.filter((id) => !pinned.includes(id)); | ||
| const items = [...pinned, ...runningNotPinned]; |
There was a problem hiding this comment.
Collapse multi-window apps to one taskbar item.
windows.map((w) => w.appId) preserves duplicates, so one app with two open windows produces two identical taskbar buttons. That also makes each duplicate target the same onAppClick path instead of representing distinct windows.
Proposed fix
- const runningAppIds = windows.map((w) => w.appId);
+ const runningAppIds = [...new Set(windows.map((w) => w.appId))];
const runningNotPinned = runningAppIds.filter((id) => !pinned.includes(id));
const items = [...pinned, ...runningNotPinned];📝 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
| const runningAppIds = windows.map((w) => w.appId); | |
| const runningNotPinned = runningAppIds.filter((id) => !pinned.includes(id)); | |
| const items = [...pinned, ...runningNotPinned]; | |
| const runningAppIds = [...new Set(windows.map((w) => w.appId))]; | |
| const runningNotPinned = runningAppIds.filter((id) => !pinned.includes(id)); | |
| const items = [...pinned, ...runningNotPinned]; |
🤖 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 `@desktop/src/components/dock/WindowsTaskbar.tsx` around lines 5 - 7, Replace
the current duplicate-preserving runningAppIds logic with a deduplicated list of
appIds so each running app produces a single taskbar item: create a Map or use
Array.from(new Set(windows.map(w => w.appId))) to derive unique runningAppIds,
and update runningNotPinned/items to use that deduplicated list; also ensure the
click handler (onAppClick) receives or resolves to a specific window id for that
app (e.g., map appId -> first or last window id from windows array or maintain a
Map<string, string[]> of appId to windowIds and pass a representative window id
when invoking onAppClick) so each taskbar item still targets a concrete window.
Comment on lines
+19
to
+23
| <div style={{ position: "fixed", zIndex: 10000, top: 4, right: 8, pointerEvents: "auto" }}> | ||
| <button | ||
| aria-label="taOS assistant" | ||
| onClick={openPanel} | ||
| className="rounded-full p-2 bg-shell-surface-active hover:brightness-110 transition-[filter]" |
There was a problem hiding this comment.
Respect mobile safe-area insets for the escape hatch.
App.tsx mounts this on mobile too, but top: 4 / right: 8 can place the button under a notch or browser chrome. That makes the one guaranteed recovery control partially hidden on exactly the devices where screen edges are constrained.
Suggested fix
- <div style={{ position: "fixed", zIndex: 10000, top: 4, right: 8, pointerEvents: "auto" }}>
+ <div
+ style={{
+ position: "fixed",
+ zIndex: 10000,
+ top: "calc(env(safe-area-inset-top, 0px) + 4px)",
+ right: "calc(env(safe-area-inset-right, 0px) + 8px)",
+ pointerEvents: "auto",
+ }}
+ >📝 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 style={{ position: "fixed", zIndex: 10000, top: 4, right: 8, pointerEvents: "auto" }}> | |
| <button | |
| aria-label="taOS assistant" | |
| onClick={openPanel} | |
| className="rounded-full p-2 bg-shell-surface-active hover:brightness-110 transition-[filter]" | |
| <div | |
| style={{ | |
| position: "fixed", | |
| zIndex: 10000, | |
| top: "calc(env(safe-area-inset-top, 0px) + 4px)", | |
| right: "calc(env(safe-area-inset-right, 0px) + 8px)", | |
| pointerEvents: "auto", | |
| }} | |
| > | |
| <button | |
| aria-label="taOS assistant" | |
| onClick={openPanel} | |
| className="rounded-full p-2 bg-shell-surface-active hover:brightness-110 transition-[filter]" |
🤖 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 `@desktop/src/components/SafetyFloor.tsx` around lines 19 - 23, The fixed
escape-hatch button in SafetyFloor is positioned with hardcoded top/right values
that can sit under notches; update the container's inline style (the div
containing the button in SafetyFloor) to account for safe-area insets using CSS
env()—for example set top and right using calc with env(safe-area-inset-top,
0px) and env(safe-area-inset-right, 0px) (e.g. top:
`calc(env(safe-area-inset-top, 0px) + 4px)` and right:
`calc(env(safe-area-inset-right, 0px) + 8px)`), so the button (opened by
openPanel) stays visible on mobile devices with notches.
Comment on lines
+129
to
+139
| export function applyThemeConfig(cfg: ThemeConfig) { | ||
| revertTheme(); | ||
| const root = document.documentElement; | ||
| for (const [k, v] of Object.entries(cfg.tokens || {})) { | ||
| if (ALLOWED_TOKENS.has(k) && typeof v === "string") { | ||
| root.style.setProperty(k, v); | ||
| _applied.push(k); | ||
| } | ||
| } | ||
| useThemeStore.setState({ structure: cfg.structure || {}, effects: cfg.effects || [] }); | ||
| } |
There was a problem hiding this comment.
Apply the theme wallpaper as part of the same flow.
cfg.wallpaper is remembered here, but never applied to the active wallpaper state. As written, preview/boot restore/keep can switch tokens, structure, and effects while leaving the previous background on screen. That breaks the advertised live preview + restore behavior for any theme that relies on its wallpaper.
Also applies to: 173-198, 204-219
🤖 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 `@desktop/src/stores/theme-store.ts` around lines 129 - 139, applyThemeConfig
currently updates tokens, structure, and effects but never applies
cfg.wallpaper; update applyThemeConfig (and the similar flows around lines
173-198 and 204-219) to set and apply the wallpaper as part of the same flow by
reading cfg.wallpaper and invoking the existing wallpaper application path
(e.g., call the function that applies wallpapers or update the wallpaper store
state such as useWallpaperStore.setState({ active: cfg.wallpaper }) or call
applyWallpaper(cfg.wallpaper)), ensuring the DOM/active wallpaper is updated
after revertTheme and before returning so theme preview/restore keeps wallpaper
in sync.
Comment on lines
+45
to
+47
| root = (_themes_root(request) / theme_id / "assets").resolve() | ||
| target = (root / path).resolve() | ||
| if not str(target).startswith(str(root) + "/") or not target.is_file(): |
There was a problem hiding this comment.
Validate theme_id before using it as the asset root.
theme_id is untrusted input here. With values like ../../.., root resolves outside data_dir/themes, and the current check only constrains path relative to that attacker-controlled root. That makes this endpoint an arbitrary file read for files under the chosen directory.
Suggested fix
`@router.get`("/api/themes/{theme_id}/assets/{path:path}")
async def theme_asset(request: Request, theme_id: str, path: str):
- root = (_themes_root(request) / theme_id / "assets").resolve()
- target = (root / path).resolve()
- if not str(target).startswith(str(root) + "/") or not target.is_file():
+ themes_root = _themes_root(request).resolve()
+ theme_root = (themes_root / theme_id).resolve()
+ target = (theme_root / "assets" / path).resolve()
+ try:
+ theme_root.relative_to(themes_root)
+ target.relative_to(theme_root / "assets")
+ except ValueError:
+ return JSONResponse({"error": "not found"}, status_code=404)
+ if not target.is_file():
return JSONResponse({"error": "not found"}, status_code=404)
return FileResponse(target)🤖 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 `@tinyagentos/routes/themes.py` around lines 45 - 47, Validate and sanitize
theme_id before using it to build root: compute base =
_themes_root(request).resolve(), then ensure theme_id does not contain path
separators or traversal segments (reject values with "/" or "\" or ".." or use
only the final path component via Path(theme_id).name), then set root = (base /
theme_id / "assets").resolve() and assert str(root).startswith(str(base) + "/")
before resolving target; keep the existing target check that target is a file
and is under root. This prevents attacker-controlled theme_id from escaping the
intended themes directory while preserving the path traversal check for path.
Comment on lines
+13
to
+19
| try: | ||
| data = yaml.safe_load(text) or {} | ||
| except yaml.YAMLError as exc: | ||
| raise ThemePackageError(f"invalid theme.yaml: {exc}") from exc | ||
| for key in _REQUIRED: | ||
| if not data.get(key): | ||
| raise ThemePackageError(f"theme manifest missing required field: {key}") |
There was a problem hiding this comment.
Reject non-object manifests before field access.
yaml.safe_load() can return a list, string, or number. In that case data.get(...) raises AttributeError, so a malformed theme.yaml becomes a 500 instead of a ThemePackageError/400.
Suggested fix
def parse_theme_manifest(text: str) -> dict:
try:
data = yaml.safe_load(text) or {}
except yaml.YAMLError as exc:
raise ThemePackageError(f"invalid theme.yaml: {exc}") from exc
+ if not isinstance(data, dict):
+ raise ThemePackageError("theme.yaml must contain a top-level mapping")
for key in _REQUIRED:
if not data.get(key):
raise ThemePackageError(f"theme manifest missing required field: {key}")🤖 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 `@tinyagentos/themes/package.py` around lines 13 - 19, After loading with
yaml.safe_load (variable data), validate that data is a mapping/dict before
accessing keys; if data is not a dict (e.g., list, str, int) raise a
ThemePackageError indicating an invalid/malformed manifest. Update the block
around yaml.safe_load in package.py to check isinstance(data, dict) (or
collections.abc.Mapping) and raise ThemePackageError with a clear message prior
to the for key in _REQUIRED loop so subsequent data.get(...) calls are safe.
Comment on lines
+41
to
+49
| theme_dir.mkdir(parents=True, exist_ok=True) | ||
| for member in zf.namelist(): | ||
| if member.endswith("/"): | ||
| continue | ||
| dest = (theme_dir / member).resolve() | ||
| if not str(dest).startswith(str(theme_dir) + "/") and dest != theme_dir: | ||
| raise ThemePackageError(f"unsafe path in package: {member}") | ||
| dest.parent.mkdir(parents=True, exist_ok=True) | ||
| dest.write_bytes(zf.read(member)) |
There was a problem hiding this comment.
Make extraction atomic or clean up on failure.
This writes into the live theme directory before the whole archive has passed validation. If a later member is unsafe, the install fails but earlier files stay on disk; those leftovers are then reachable through the asset-serving route even though the theme never finished installing.
Extract into a temporary directory under themes_root, validate every member, then rename into place only after success. At minimum, remove theme_dir on any exception from the extraction loop.
🤖 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 `@tinyagentos/themes/package.py` around lines 41 - 49, The extraction currently
writes files directly into theme_dir while iterating over zf.namelist(), leaving
partial files if a later member fails; change install logic to extract into a
temporary directory inside themes_root (e.g., create temp_dir next to
theme_dir), perform the same validation checks (member.endswith("/"),
path-safety check against temp_dir, dest.parent.mkdir, write_bytes) against that
temp_dir, and only atomically rename/move temp_dir to theme_dir after all
members succeed; additionally ensure any exception during extraction removes the
temp_dir (or on failure fall back to cleanup) and keep raising ThemePackageError
for unsafe paths as before so partial installs are never served.
Comment on lines
+64
to
+69
| for surface, conf in structure.items(): | ||
| if surface not in _VARIANTS: | ||
| raise ThemeError(f"unknown structural surface: {surface}") | ||
| variant = (conf or {}).get("variant") | ||
| if variant is not None and variant not in _VARIANTS[surface]: | ||
| raise ThemeError(f"unknown {surface} variant: {variant}") |
There was a problem hiding this comment.
Validate each structural config before reading variant.
A payload like {"structure": {"dock": "macos-dock"}} will hit .get("variant") on a str and raise AttributeError, turning invalid user input into a 500 instead of a ThemeError.
Suggested fix
for surface, conf in structure.items():
if surface not in _VARIANTS:
raise ThemeError(f"unknown structural surface: {surface}")
- variant = (conf or {}).get("variant")
+ if conf is None:
+ conf = {}
+ if not isinstance(conf, dict):
+ raise ThemeError(f"{surface} config must be an object")
+ variant = conf.get("variant")
if variant is not None and variant not in _VARIANTS[surface]:
raise ThemeError(f"unknown {surface} variant: {variant}")🤖 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 `@tinyagentos/themes/schema.py` around lines 64 - 69, The loop over structure
items currently assumes each structural config (conf) is a mapping and calls
conf.get("variant"), which raises AttributeError for non-dict values (e.g., a
string) and causes a 500; update the validation inside the for surface, conf in
structure.items() loop to first ensure conf is either None or a dict/mapping
before accessing conf.get — if conf is of the wrong type, raise ThemeError with
a clear message like "invalid structural config for {surface}"; then continue to
check variant against _VARIANTS[surface] as before.
Comment on lines
+89
to
+94
| requires = set(cfg.get("requires", []) or []) | _SAFETY_FLOOR | ||
| out = dict(cfg) | ||
| out["tokens"] = tokens | ||
| out["structure"] = structure | ||
| out["effects"] = effects | ||
| out["requires"] = sorted(requires) |
There was a problem hiding this comment.
Reject non-list requires values before normalizing them.
set(cfg.get("requires", []) or []) silently accepts the wrong shape. For example, "assistant" becomes a set of characters, so malformed configs are normalized into garbage capability names instead of being rejected.
Suggested fix
- requires = set(cfg.get("requires", []) or []) | _SAFETY_FLOOR
+ raw_requires = cfg.get("requires", []) or []
+ if not isinstance(raw_requires, list) or not all(isinstance(item, str) for item in raw_requires):
+ raise ThemeError("requires must be a list of strings")
+ requires = set(raw_requires) | _SAFETY_FLOOR📝 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
| requires = set(cfg.get("requires", []) or []) | _SAFETY_FLOOR | |
| out = dict(cfg) | |
| out["tokens"] = tokens | |
| out["structure"] = structure | |
| out["effects"] = effects | |
| out["requires"] = sorted(requires) | |
| raw_requires = cfg.get("requires", []) or [] | |
| if not isinstance(raw_requires, list) or not all(isinstance(item, str) for item in raw_requires): | |
| raise ThemeError("requires must be a list of strings") | |
| requires = set(raw_requires) | _SAFETY_FLOOR | |
| out = dict(cfg) | |
| out["tokens"] = tokens | |
| out["structure"] = structure | |
| out["effects"] = effects | |
| out["requires"] = sorted(requires) |
🤖 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 `@tinyagentos/themes/schema.py` around lines 89 - 94, The code currently builds
requires via set(cfg.get("requires", []) or []) which accepts strings (turning
them into sets of chars); change it to first fetch raw = cfg.get("requires",
None), normalize None to an empty list, and explicitly reject any non-list/tuple
input by raising a clear ValueError (or TypeError) indicating "requires must be
a list". After that, compute requires = set(raw) | _SAFETY_FLOOR and continue
setting out["requires"] = sorted(requires); reference the local names cfg,
requires, _SAFETY_FLOOR, and out to locate and update this validation.
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.
Implements the Theme Manager Phase 1 (spec/plan:
docs/superpowers/specs|plans/2026-05-31-theme-manager-*). Lets users (and, once the assistant gains tool-calling, the agent) completely restyle the desktop as a shareable theme — with a hard safety floor that no theme can break.What's in it
Backend (
tinyagentos/themes/):schema.py— theme vocabulary +validate_theme_config(): known token keys only, values sanitised (nourl()/expression()/JS/@import, incl. the CSS-commenturl/**/()bypass), known structural variants + effect modules only, per-effect param allowlists (cursor restricted to safe keywords), and a non-negotiable safety floor (assistant+launcheralways required).package.py—.taosthemeparse/extract, path-jailed.store.py—ThemeStore.routes/themes.py— install/list/remove/assets.routes/mcp.py— agent theme tools (get_theme_schema/create_theme/preview_theme) as REST endpoints.Frontend (
desktop/):theme-store.ts): writes allowlisted token CSS-vars to:root(defence-in-depth — client re-filters), publishes structure/effects, fully revertible; per-theme wallpaper memory; restore-active-theme-on-boot.theme/effects/): crt/scanlines/glow/cursor in apointer-events:nonelayer.components/dock/): macos-dock + windows-taskbar over shared pinned-app data (the structural-variant proof).SafetyFloor.tsx): system-owned, always-mounted assistant button at z-index 10000 — a hostile theme can't hide or block it (regression-tested).SettingsApp → Themes), built-in Default + Matrix Terminal themes.Safety (final review pass + fixes)
Reviewed: validation airtight, apply-engine defence-in-depth, safety floor structural (survives a theme hiding everything), package path-jailed, backend↔frontend token allowlists identical. Three review findings fixed before this PR (effect-param validation, CSS-comment url bypass, boot restore).
Tests
20 backend (
tests/themes/) + 119 frontend (theme/stores/dock/safety) passing; tsc clean; build OK.Known scope (deferred, per spec)
Worth a live look at the Matrix Terminal theme + the Settings → Themes preview/keep/revert before merge.
Summary by CodeRabbit
Release Notes
New Features
Improvements