Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
Coverage Report
|
||||||||||||||||||||||||||||||||||||||||||||||||||
🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get PR details
gh pr view 81 --repo rtuszik/KoalaKeys
echo "---FILES CHANGED---"
gh pr diff 81 --repo rtuszik/KoalaKeys --name-only
echo "---PR COMMENTS---"
gh pr view 81 --repo rtuszik/KoalaKeys --commentsLength of output: 31529 🏁 Script executed: #!/bin/bash
# Check pyproject.toml for scripts/entry points
cat pyproject.toml
echo "---README---"
cat README.md | head -100Length of output: 5139
Outstanding TODOs
Other things to check
Would you like me to do a full review of all the changes now? ✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughAdds a full theming system ( ChangesTheming System and Package Restructure
CI Hardening and Tooling
Estimated code review effort🎯 5 (Critical) | ⏱️ ~110 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches⚔️ Resolve merge conflicts
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.0)src/koalakeys/templates/cheatsheets/scripts/main.jsFile contains syntax errors that prevent linting: Line 75: Expected a property, a shorthand property, a getter, a setter, or a method but instead found '{ keyboard_layout | tojson; Line 122: Expected a statement but instead found '} 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pyproject.toml (2)
3-3:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign package version with the 1.0.0 release intent.
Line 3 still declares
0.6.0, which conflicts with the stated release target and can break release automation and artefact labelling.Proposed change
-version = "0.6.0" +version = "1.0.0"
1-8:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a CLI script entry for
uv run koalakeys.
[project.scripts]is missing, so theuv run koalakeysobjective will not work.Proposed change
[project] name = "koalakeys" version = "0.6.0" description = "Build interactive, portable cheatsheets from YAML." readme = "README.md" requires-python = ">=3.9" dependencies = ["jinja2>=3.1.6", "python-dotenv>=1.1.1", "ruamel-yaml>=0.18.14"] + +[project.scripts] +koalakeys = "koalakeys.<module_with_main>:main"
🧹 Nitpick comments (2)
.github/workflows/pytest.yml (1)
18-20: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRemove the duplicated non-browser test run.
Line 18-Line 20 already runs pytest, then Line 32-Line 38 runs it again with coverage. Keeping only the coverage run avoids duplicate CI time.
Proposed change
- - run: | - uv run pytest - # Browser/Playwright tests are excluded by default (pytest.ini) and run # only on pull requests, where we install Chromium for them. - name: Install Playwright browser if: github.event_name == 'pull_request' run: | uv run playwright install --with-deps chromiumAlso applies to: 32-38
tests/conftest.py (1)
9-16: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsolidate
isolated_outputinto one fixture definition.Line 9 introduces a shared fixture, but there is also an
isolated_outputfixture intests/test_generate_cheatsheet.py(Lines 10-16 in graph context). Keeping both increases drift risk and can create subtle shadowing differences; prefer using only theconftest.pyfixture.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f15c7b39-fc7a-4cef-ac6b-5dc0bba20052
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (45)
.github/actions/setup-python/action.yml.github/workflows/lint.yml.github/workflows/pytest.yml.pre-commit-config.ymldocs/schema/cheatsheet.schema.jsonpyproject.tomlpytest.inisrc/koalakeys/__init__.pysrc/koalakeys/generate_cheatsheet.pysrc/koalakeys/layouts/keyboard_layouts.yamlsrc/koalakeys/layouts/system_mappings.yamlsrc/koalakeys/logger.pysrc/koalakeys/template_renderer.pysrc/koalakeys/templates/base.htmlsrc/koalakeys/templates/cheatsheets/assets/cheatsheets.csssrc/koalakeys/templates/cheatsheets/cheatsheet-template.htmlsrc/koalakeys/templates/cheatsheets/components/body.htmlsrc/koalakeys/templates/cheatsheets/scripts/main.jssrc/koalakeys/templates/index/assets/index.csssrc/koalakeys/templates/index/components/body.htmlsrc/koalakeys/templates/index/components/footer.htmlsrc/koalakeys/templates/index/index_template.htmlsrc/koalakeys/templates/index/scripts/main.jssrc/koalakeys/themes/catppuccin.yamlsrc/koalakeys/themes/dracula.yamlsrc/koalakeys/themes/gruvbox.yamlsrc/koalakeys/themes/nord.yamlsrc/koalakeys/themes/rose-pine.yamlsrc/koalakeys/themes/solarized.yamlsrc/koalakeys/theming.pysrc/koalakeys/validate_yaml.pysrc/layouts/keyboard_layouts.yamlsrc/layouts/system_mappings.yamlsrc/templates/cheatsheets/assets/cheatsheets.csssrc/templates/cheatsheets/cheatsheet-template.htmlsrc/templates/index/assets/index.csstests/__init__.pytests/conftest.pytests/test_browser.pytests/test_generate_cheatsheet.pytests/test_integration.pytests/test_replace_shortcut_names.pytests/test_template_renderer.pytests/test_theming.pytests/test_validate_yaml.py
💤 Files with no reviewable changes (6)
- src/templates/index/assets/index.css
- tests/init.py
- src/layouts/keyboard_layouts.yaml
- src/templates/cheatsheets/cheatsheet-template.html
- src/layouts/system_mappings.yaml
- src/templates/cheatsheets/assets/cheatsheets.css
| THEMES_DIR = PROJECT_ROOT / "themes" | ||
| STYLES_DIR = PROJECT_ROOT / "styles" | ||
| LAYOUTS_DIR = PACKAGE_DIR / "layouts" |
There was a problem hiding this comment.
Theme lookup path is inconsistent with the relocated package assets.
Line 29 points to PROJECT_ROOT / "themes", but this PR moves themes into the package namespace. That can cause resolve_theme(...) to miss theme files and make generate_html(...) return None on otherwise valid input.
💡 Proposed fix
-THEMES_DIR = PROJECT_ROOT / "themes"
+THEMES_DIR = PACKAGE_DIR / "themes"| margin-bottom: 10px; | ||
| font-weight: 700; | ||
| font-size: 2.2rem; | ||
| position: absolute; | ||
| left: 50%; | ||
| right: 0; | ||
| margin: 0; | ||
| white-space: nowrap; | ||
| overflow: hidden; | ||
| text-overflow: ellipsis; | ||
| width: 100vw; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "cheatsheets.css" -type fRepository: rtuszik/KoalaKeys
Length of output: 122
🏁 Script executed:
cat -n src/koalakeys/templates/cheatsheets/assets/cheatsheets.css | sed -n '25,50p'Repository: rtuszik/KoalaKeys
Length of output: 776
🏁 Script executed:
cat -n src/koalakeys/templates/cheatsheets/assets/cheatsheets.css | sed -n '110,165p'Repository: rtuszik/KoalaKeys
Length of output: 1438
🌐 Web query:
CSS word-wrap property deprecated MDN
💡 Result:
CSS word-wrap is not a separate modern property; it’s a legacy name/alias of overflow-wrap, so it is effectively deprecated in favor of overflow-wrap. MDN’s overflow-wrap documentation notes that the property was originally a nonstandard Microsoft extension called “word-wrap” and that it has since been renamed to overflow-wrap, with “word-wrap” being an alias. [1] The CSS Text Module Level 3 specification similarly states that UAs must treat word-wrap as a legacy name alias of overflow-wrap. [2] So: prefer overflow-wrap (e.g., overflow-wrap: anywhere / break-word) rather than word-wrap. [1][2]
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Properties/overflow-wrap
- 2: https://www.w3.org/TR/css-text-3/
Remove redundant margin and deprecated word-wrap declarations.
The h1 rule has margin-bottom: 10px; on line 31 immediately overridden by margin: 0; on line 37. The .shortcut-key (lines 117–118) and .shortcut-description (lines 158–159) blocks both declare the legacy word-wrap alias alongside the modern overflow-wrap property. Remove the dead declarations to reduce lint noise and improve code clarity.
Suggested patch
h1 {
color: var(--kk-accent);
text-align: center;
- margin-bottom: 10px;
font-weight: 700;
font-size: 2.2rem;
position: absolute;
left: 50%;
right: 0;
margin: 0;
@@
.shortcut-key {
@@
- word-wrap: break-word;
overflow-wrap: break-word;
@@
.shortcut-description {
@@
- word-wrap: break-word;
overflow-wrap: break-word;🧰 Tools
🪛 Stylelint (17.13.0)
[error] 37-37: Overridden property "margin-bottom" by shorthand "margin" (declaration-block-no-shorthand-property-overrides)
(declaration-block-no-shorthand-property-overrides)
Source: Linters/SAST tools
| #dark-mode-toggle { | ||
| background: none; | ||
| border: none; | ||
| cursor: pointer; | ||
| font-size: 24px; | ||
| color: var(--text); | ||
| transition: color 0.3s; | ||
| } | ||
|
|
||
| body.dark-mode #dark-mode-toggle { | ||
| color: var(--mocha-text); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/koalakeys/templates/index/assets/index.css | sed -n '95,115p'Repository: rtuszik/KoalaKeys
Length of output: 546
🏁 Script executed:
cat -n src/koalakeys/templates/index/assets/index.css | head -50Repository: rtuszik/KoalaKeys
Length of output: 1679
🏁 Script executed:
rg "(?:--text|--latte-text|:root)" src/koalakeys/templates/index/assets/index.css -A 2 -B 2Repository: rtuszik/KoalaKeys
Length of output: 846
Use the defined --latte-text token for the toggle colour in light mode.
Line 104 references var(--text), which is not defined in the CSS variables. This causes the colour to depend on fallback or inheritance rather than the intended design token. Replace it with --latte-text to match the pattern used elsewhere in the stylesheet and the corresponding dark-mode rule.
Suggested patch
`#dark-mode-toggle` {
background: none;
border: none;
cursor: pointer;
font-size: 24px;
- color: var(--text);
+ color: var(--latte-text);
transition: color 0.3s;
}📝 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.
| #dark-mode-toggle { | |
| background: none; | |
| border: none; | |
| cursor: pointer; | |
| font-size: 24px; | |
| color: var(--text); | |
| transition: color 0.3s; | |
| } | |
| body.dark-mode #dark-mode-toggle { | |
| color: var(--mocha-text); | |
| } | |
| `#dark-mode-toggle` { | |
| background: none; | |
| border: none; | |
| cursor: pointer; | |
| font-size: 24px; | |
| color: var(--latte-text); | |
| transition: color 0.3s; | |
| } | |
| body.dark-mode `#dark-mode-toggle` { | |
| color: var(--mocha-text); | |
| } |
| if key in RESERVED_KEYS: | ||
| if key in MODES and value: | ||
| modes[key] = {k: _validate_token_value(k, v) for k, v in value.items()} | ||
| continue |
There was a problem hiding this comment.
Guard mode blocks before iterating .items()
Line 147 can raise an uncaught exception when a theme defines light/dark as a non-mapping value. This bypasses ThemeError handling and can crash generation.
Suggested fix
if key in RESERVED_KEYS:
if key in MODES and value:
- modes[key] = {k: _validate_token_value(k, v) for k, v in value.items()}
+ if not isinstance(value, dict):
+ raise ThemeError(f"Theme mode '{key}' must be a mapping of token -> value")
+ modes[key] = {k: _validate_token_value(k, v) for k, v in value.items()}
continue| if narrowed is not None: | ||
| if not isinstance(narrowed, list) or not set(narrowed) <= set(MODES): | ||
| raise ThemeError(f"User theme '{name}': 'modes' must be a subset of {list(MODES)}") |
There was a problem hiding this comment.
Harden modes narrowing type checks
Line 224 can throw TypeError for unhashable entries (for example nested lists) before you raise ThemeError. Validate element types explicitly first.
Suggested fix
if narrowed is not None:
- if not isinstance(narrowed, list) or not set(narrowed) <= set(MODES):
+ if (
+ not isinstance(narrowed, list)
+ or not all(isinstance(mode, str) for mode in narrowed)
+ or not set(narrowed) <= set(MODES)
+ ):
raise ThemeError(f"User theme '{name}': 'modes' must be a subset of {list(MODES)}")📝 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.
| if narrowed is not None: | |
| if not isinstance(narrowed, list) or not set(narrowed) <= set(MODES): | |
| raise ThemeError(f"User theme '{name}': 'modes' must be a subset of {list(MODES)}") | |
| if narrowed is not None: | |
| if ( | |
| not isinstance(narrowed, list) | |
| or not all(isinstance(mode, str) for mode in narrowed) | |
| or not set(narrowed) <= set(MODES) | |
| ): | |
| raise ThemeError(f"User theme '{name}': 'modes' must be a subset of {list(MODES)}") |
| validators = [ | ||
| validate_required_keys, | ||
| validate_title, | ||
| validate_render_options, | ||
| validate_styling, | ||
| validate_layout, | ||
| validate_shortcuts, | ||
| ] | ||
| results = [validator(data) for validator in validators] | ||
| is_valid = all(results) |
There was a problem hiding this comment.
Add a top-level mapping guard before running validator list
The new aggregated execution can crash on non-object YAML (for example a list), because validators like validate_render_options call data.get(...). Fail fast with a type check before invoking validators.
Suggested fix
if data is None:
logger.error(f"Empty YAML file: {file_path}")
return False
+ if not isinstance(data, dict):
+ logger.error(f"Top-level YAML must be a mapping/object: {file_path}")
+ return False
validators = [
validate_required_keys,
validate_title,
validate_render_options,📝 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.
| validators = [ | |
| validate_required_keys, | |
| validate_title, | |
| validate_render_options, | |
| validate_styling, | |
| validate_layout, | |
| validate_shortcuts, | |
| ] | |
| results = [validator(data) for validator in validators] | |
| is_valid = all(results) | |
| if data is None: | |
| logger.error(f"Empty YAML file: {file_path}") | |
| return False | |
| if not isinstance(data, dict): | |
| logger.error(f"Top-level YAML must be a mapping/object: {file_path}") | |
| return False | |
| validators = [ | |
| validate_required_keys, | |
| validate_title, | |
| validate_render_options, | |
| validate_styling, | |
| validate_layout, | |
| validate_shortcuts, | |
| ] | |
| results = [validator(data) for validator in validators] | |
| is_valid = all(results) |
| class TestCheatsheetInBrowser: | ||
| def test_loads_without_console_errors(self, page): | ||
| assert page.console_errors == [], f"unexpected JS errors: {page.console_errors}" | ||
| assert "Full Featured Test" in page.title() or page.locator("h1").count() >= 0 |
There was a problem hiding this comment.
Fix the always-true fallback in the page title assertion.
Line 56 uses page.locator("h1").count() >= 0, which always passes and can hide a broken render path.
Proposed fix
- assert "Full Featured Test" in page.title() or page.locator("h1").count() >= 0
+ assert "Full Featured Test" in page.title() or page.locator("h1").count() > 0📝 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.
| assert "Full Featured Test" in page.title() or page.locator("h1").count() >= 0 | |
| assert "Full Featured Test" in page.title() or page.locator("h1").count() > 0 |
1 participant
TODO:
uv run koalakeys