test(theme): cover builtin theme registry - #1073
Open
mydd7 wants to merge 1 commit into
Open
Conversation
listBuiltinThemes, getBuiltinTheme and getDefaultTheme back the theme system but had no tests. Lock the registry invariants that catch registration mistakes: a non-empty list, a unique id per theme, every listed theme resolvable by its id, undefined for an unknown id, and the default theme resolving to a registered theme (DEFAULT_THEME_ID is present) rather than the BUILTIN[0] fallback.
📝 WalkthroughWalkthroughAdds Vitest coverage for the builtin theme registry, including theme listing, unique IDs, ID-based lookup, unknown IDs, and default-theme resolution. ChangesBuiltin theme registry
Estimated code review effort: 1 (Trivial) | ~3 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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: 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/modules/theme/themes/index.test.ts`:
- Around line 2-3: Update the imports in the theme tests to use the frontend
`@/`... path alias for DEFAULT_THEME_ID from ../types and getBuiltinTheme,
getDefaultTheme, and listBuiltinThemes from ./index, removing these relative
module imports while preserving the imported symbols.
- Around line 25-29: Update the default-theme test to compare getDefaultTheme()
directly with getBuiltinTheme(DEFAULT_THEME_ID), ensuring it verifies the
registered-theme lookup path rather than only the ID. Add a focused assertion
for fallback behavior when the default lookup is missing only if that precedence
is an explicit contract.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cacad1d6-d980-43c3-94ec-6a321da6aecf
📒 Files selected for processing (1)
src/modules/theme/themes/index.test.ts
Comment on lines
+2
to
+3
| import { DEFAULT_THEME_ID } from "../types"; | ||
| import { getBuiltinTheme, getDefaultTheme, listBuiltinThemes } from "./index"; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the frontend path alias for these imports.
Replace ../types and ./index with the corresponding @/... imports.
As per coding guidelines, frontend imports must use the @/... alias and must not use relative imports across modules.
🤖 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/modules/theme/themes/index.test.ts` around lines 2 - 3, Update the
imports in the theme tests to use the frontend `@/`... path alias for
DEFAULT_THEME_ID from ../types and getBuiltinTheme, getDefaultTheme, and
listBuiltinThemes from ./index, removing these relative module imports while
preserving the imported symbols.
Source: Coding guidelines
Comment on lines
+25
to
+29
| it("resolves the default theme to a registered theme, not the fallback", () => { | ||
| const ids = listBuiltinThemes().map((t) => t.id); | ||
| expect(ids).toContain(DEFAULT_THEME_ID); | ||
| expect(getDefaultTheme().id).toBe(DEFAULT_THEME_ID); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate theme files =="
fd -a 'index\.(ts|tsx)$' src/modules || true
fd -a 'themes' src/modules || true
echo "== target test context =="
if [ -f src/modules/theme/themes/index.test.ts ]; then
nl -ba src/modules/theme/themes/index.test.ts | sed -n '1,140p'
fi
echo "== find getDefaultTheme/getBuiltinTheme definitions =="
rg -n "getDefaultTheme|getBuiltinTheme|listBuiltinThemes|DEFAULT_THEME_ID|BUILTIN" src/modules/theme/themes src/modules/theme -S || trueRepository: crynta/terax-ai
Length of output: 1472
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== outline target module =="
ast-grep outline src/modules/theme/themes/index.ts --view expanded || true
echo "== relevant implementation =="
nl -ba src/modules/theme/themes/index.ts | sed -n '1,220p'
echo "== import shape in tests =="
nl -ba src/modules/theme/themes/index.test.ts | sed -n '1,220p'Repository: crynta/terax-ai
Length of output: 543
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target test context =="
sed -n '1,140p' src/modules/theme/themes/index.test.ts
echo "== implementation context =="
sed -n '1,120p' src/modules/theme/themes/index.ts
echo "== usages of default theme functions =="
rg -n "getDefaultTheme\(|getBuiltinTheme\(|listBuiltinThemes\(|DEFAULT_THEME_ID" .
echo "== static probe of implementation shape =="
python3 - <<'PY'
from pathlib import Path
path = Path("src/modules/theme/themes/index.ts")
text = path.read_text()
for needle in ["export function listBuiltinThemes", "export function getBuiltinTheme", "export function getDefaultTheme", "BUILTIN: Theme[]"]:
idx = text.find(needle)
start = max(0, text.rfind("\n", 0, idx) or idx)
body = text[idx:text.find("\nexport function", idx + 1)]
print(f"--- {needle} ---")
print(body[:1000])
PYRepository: crynta/terax-ai
Length of output: 5857
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== teraxDefault theme shape =="
sed -n '1,120p' src/modules/theme/themes/terax-default.ts
echo "== programmatic proof of fallback mask with current registry and target test =="
python3 <<'PY'
from pathlib import Path
text = Path("src/modules/theme/themes/index.ts").read_text()
for name in ["teraxDefault", "cola", "claude", "kanagawa", "tokyoNight"]:
path = f"src/modules/theme/themes/{name}.ts"
if path.exists():
body = Path(path).read_text()
for line in body.splitlines():
if "export default" in line:
print(f"{path}: {line.strip()}")
break
def id_of(name):
return next(l.strip() for l in Path(f"src/modules/theme/themes/{name}.ts").read_text().splitlines() if "id:" in l).split(sep=":", maxsplit=1)[1].strip().strip('"')
BUILTIN = list(["teraxDefault", "cola", "claude", "kanagawa", "tokyoNight"])
DEFAULT_THEME_ID = id_of("teraxDefault")
BY_ID = {i: name for name, i in enumerate(BUILTIN)}
def get_default_theme_current():
return BY_ID.get(DEFAULT_THEME_ID, BUILTIN[0])
def always_fallback(theme_id):
return BUILTIN[0]
current_id = BUILTIN[get_default_theme_current()]
fallback_id = BUILTIN[always_fallback(DEFAULT_THEME_ID)]
print("current DEFAULT_THEME_ID:", DEFAULT_THEME_ID)
print("default lookup found:", current_id == DEFAULT_THEME_ID)
print("always fallback returned:", fallback_id)
print("current test only checks returned id equals DEFAULT_THEME_ID:", current_id == current_id == DEFAULT_THEME_ID)
print("always fallback test would pass:", fallback_id == current_id == DEFAULT_THEME_ID)
print("identity guard would fail for always fallback:", current_id != fallback_id)
PYRepository: crynta/terax-ai
Length of output: 700
Make the default-theme test assert the lookup path, not only the returned ID.
getDefaultTheme() falls back to BUILTIN[0], so comparing the result to DEFAULT_THEME_ID would also pass if the lookup were removed while terax-default stays first. Compare getDefaultTheme() with getBuiltinTheme(DEFAULT_THEME_ID) and add a small separate assertion for fallback behavior if missing lookup precedence is part of the contract.
🤖 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/modules/theme/themes/index.test.ts` around lines 25 - 29, Update the
default-theme test to compare getDefaultTheme() directly with
getBuiltinTheme(DEFAULT_THEME_ID), ensuring it verifies the registered-theme
lookup path rather than only the ID. Add a focused assertion for fallback
behavior when the default lookup is missing only if that precedence is an
explicit contract.
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.
What
Adds unit tests for the builtin theme registry in
theme/themes/index.ts.Test-only.
Why
listBuiltinThemes,getBuiltinThemeandgetDefaultThemeback the themesystem, and a registration mistake (a duplicate theme id, or a
DEFAULT_THEME_IDthat no theme actually uses) would silently degrade the default without any
compile error. Nothing tested those invariants.
How
Pure registry functions, tested directly.
Locked behavior:
getBuiltinTheme, and anunknown id returns undefined.
getDefaultThemeresolves to a registered theme:DEFAULT_THEME_IDis presentin the registry, so the result is the real default and not the
BUILTIN[0]fallback.
Testing
Ran the full suite plus type-check and lint locally on Windows.
pnpm lintclean (unchanged: 103 pre-existing warnings, none introduced)pnpm check-typescleanpnpm testclean (the new suite passes; see reviewer note on onepre-existing suite)
src-tauri/) cargo clippy - N/A, no Rust changessrc-tauri/) cargo nextest - N/A, no Rust changes#[tauri::command]signature) - N/Apnpm tauri dev- N/A, no UI changeScreenshots / GIFs
N/A - no UI change.
Notes for reviewer
mainon purpose (my token lacksthe
workflowscope). Only adds a new file; should merge cleanly.src/app/eager-budget.test.tsfails locally under Windows with
git core.autocrlf=true; the committed blob isLF and it passes on CI.
Summary by CodeRabbit