Repository navigation
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 theme-aware cheat-sheet generation with six built-in themes and user-theme inheritance. Adds ChangesKoalaKeys theming and workflow overhaul
Priority: ➖ Normal Estimated code review effort: 5 (Critical) | ~110 minutes Change: Feature Merge Risk: 🟡 Moderate · up to Several CLI, security, release-metadata, and documentation defects should be corrected before merging the 1.0.0 release. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
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 |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟠 Major · Align the package version with the 1.0.0 release target. · pyproject.toml:3
pyproject.toml:3
🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAlign the package version with the 1.0.0 release target.
A build from this PR will declare version
0.6.0in its package metadata. Setversionto1.0.0, or align the release target with0.6.0, before publishing.
🟠 Major · Confine custom_css to STYLES_DIR. · generate_cheatsheet.py:142
src/koalakeys/generate_cheatsheet.py:142
🔒 Security & Privacy | 🛡️ Detected with Advanced Tier | 🟠 Major | ⚡ Quick winPath Traversal
Exploitability: Moderate
CWE: CWE-22 — Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal')Confine
custom_csstoSTYLES_DIR.If an attacker can supply a cheatsheet,
custom_csscan contain an absolute path or..segments.STYLES_DIR / filenamecan then read any readable file outsidestyles/, and the content is inserted into the generated HTML. Resolve the root and candidate paths, then reject candidates outside the resolvedSTYLES_DIRbefore callingread_text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: c1f8a17f-2c12-4f4f-a227-b9c9220a260a
⛔ Files ignored due to path filters (1)
assets/images/KoalaKeys_Example.pngis excluded by!**/*.png
📒 Files selected for processing (12)
README.mdpyproject.tomlsrc/koalakeys/cli.pysrc/koalakeys/generate_cheatsheet.pysrc/koalakeys/scaffold/__init__.pysrc/koalakeys/scaffold/example.yamlsrc/koalakeys/theming.pysrc/koalakeys/validate_yaml.pytests/test_cli.pytests/test_generate_cheatsheet.pytests/test_scaffold.pyyaml_cheatsheet_spec.md
💤 Files with no reviewable changes (1)
- src/koalakeys/validate_yaml.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/koalakeys/theming.py
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.
| KoalaKeys generates and organizes portable, interactive HTML cheat sheets for keyboard shortcuts. It's designed for developers, designers, and power users who want to keep their essential shortcuts easily accessible. | ||
|
|
||
| > **Quick Start**: To create a cheat sheet, add a YAML file to the `cheatsheets` directory and run `python src/generate_cheatsheet.py`. For detailed YAML formatting instructions, see the [YAML Cheat Sheet Specification Guide](yaml_cheatsheet_spec.md). | ||
| > **Quick Start**: Run `uvx koalakeys init` to scaffold a project, add or edit YAML files in its `cheatsheets/` directory, then run `koalakeys generate`. For detailed YAML formatting instructions, see the [YAML Cheat Sheet Specification Guide](yaml_cheatsheet_spec.md). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the quick-start directory explicit.
uvx koalakeys init creates ./koalakeys by default, while generate reads cheatsheets/ from the current directory. The documented sequence fails if the user runs koalakeys generate from the parent directory. Add cd koalakeys before generation, and use uvx koalakeys generate or install the tool before using the bare command.
| print(f"Validating {path}...") | ||
| if not validate_yaml(path): | ||
| all_valid = False | ||
| for warning in lint_yaml(path): |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Check that the validation path is a file before linting it.
cmd_validate calls lint_yaml(path) after validate_yaml(path) reports failure. For a missing path or directory, lint_yaml can raise an uncaught FileNotFoundError or IsADirectoryError. Check that path is a file before calling lint_yaml, so the command returns status 1 without a traceback.
| if not cheatsheets: | ||
| print("No valid cheatsheets were generated due to errors.") | ||
| return 1 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Return failure when any cheatsheet generation fails.
If one YAML file fails and another succeeds, cheatsheets is non-empty and Line 276 returns 0. The command then reports success while omitting the failed cheatsheet from the index. Track per-file failures and return a non-zero status when any selected file fails.
This branch has not been deployed
1 participant
TODO:
uv run koalakeys