refined tray icon and added animations - #1
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThis PR replaces the port-killer status icon with a dot-matrix display, adds GNOME extension rendering/style modules, updates extension asset installation, revises Waybar icon/CSS handling, and formats the Waybar count text as zero-padded two digits. ChangesDot-matrix visualization feature
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant PortKillerIndicator
participant StylesModule
participant DotMatrixModule
User->>PortKillerIndicator: right-click
PortKillerIndicator->>PortKillerIndicator: cycle persisted style index
PortKillerIndicator->>StylesModule: decodeStyleIndex(flatIndex)
StylesModule-->>PortKillerIndicator: animation + color scheme
PortKillerIndicator->>StylesModule: resolveColors(scheme, themeFg)
StylesModule-->>PortKillerIndicator: digit/accent/trail colors
PortKillerIndicator->>DotMatrixModule: digitDotPositions(tens, ones)
DotMatrixModule-->>PortKillerIndicator: dot coordinates
PortKillerIndicator->>PortKillerIndicator: repaint drawing area
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/setup.rs (1)
341-355: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMigration path can corrupt user's style.css with orphaned CSS lines.
Previously, content containing
port-killer-server.svgreturned early (no-op). Now that marker also triggers the removal branch at Line 345, which strips lines via.filter(|line| !line.contains("custom-port-killer")). This only removes selector lines (e.g.#custom-port-killer {,#custom-port-killer.active {) but leaves their property lines and closing}behind as stray top-level CSS, since those don't contain the literal substring"custom-port-killer". This is invalid CSS that can break GTK's stylesheet parsing beyond just this widget for anyone upgrading from a prior server-icon install.🔧 Proposed fix: track brace depth to remove the whole rule block
- if content.contains("`#custom-port-killer`") || content.contains("port-killer-server.svg") { - let without_old: String = content - .lines() - .filter(|line| !line.contains("custom-port-killer")) - .collect::<Vec<_>>() - .join("\n"); + if content.contains("`#custom-port-killer`") || content.contains("port-killer-server.svg") { + let mut without_old = String::new(); + let mut depth: i32 = 0; + let mut in_old_rule = false; + for line in content.lines() { + let starts_rule = line.contains("custom-port-killer") && depth == 0; + if starts_rule { + in_old_rule = true; + } + if in_old_rule { + depth += line.matches('{').count() as i32; + depth -= line.matches('}').count() as i32; + if depth <= 0 { + in_old_rule = false; + depth = 0; + } + continue; + } + without_old.push_str(line); + without_old.push('\n'); + }Recommend adding a regression test that runs
patch_waybar_styleagainst a realistic pre-existing multi-line old CSS block and asserts the resulting file is valid/parseable, not just checkingWAYBAR_CSSconstant contents.🤖 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/setup.rs` around lines 341 - 355, The `patch_waybar_style` migration branch is only filtering lines containing `custom-port-killer`, which leaves orphaned property lines and closing braces behind when replacing an old `port-killer-server.svg` style block. Update the removal logic in `setup.rs` so it removes the entire legacy `#custom-port-killer` rule(s) using brace-depth or another block-aware approach before appending `WAYBAR_CSS`, and keep the early return for `port-killer-matrix-sprite.svg` unchanged. Add a regression test around `patch_waybar_style` that feeds in a realistic multi-line old stylesheet and verifies the rewritten output is valid rather than only matching constants.
🧹 Nitpick comments (3)
src/main.rs (1)
230-248: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: hardcoded
"00"duplicates the padding logic.The empty branch hardcodes
"00".to_string()while the non-empty branch derives the padded string fromcount. Sincecountis0in the empty case anyway, usingformat!("{count:02}")in both branches would avoid duplicating the "2-digit zero pad" convention in two places.🤖 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/main.rs` around lines 230 - 248, The empty branch in the listener display logic duplicates the zero-padding rule by hardcoding "00" instead of using the same count formatting used in the non-empty path. Update the `main` status text construction so the `count` value is rendered consistently with `format!("{count:02}")` in both branches, keeping the `listeners.is_empty()` handling and tooltip/class selection unchanged.src/setup.rs (1)
401-407: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider adding a migration test for
patch_waybar_style.Current test only checks
WAYBAR_CSSstring contents; there's no test exercising the migration branch (Lines 341-355) against a realistic old-style CSS block, which would have caught the orphaned-line issue flagged above.🤖 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/setup.rs` around lines 401 - 407, Add a migration test for patch_waybar_style that feeds it a realistic old-style Waybar CSS block and asserts the migrated output is correct. The current waybar_css_includes_matrix_animation test only checks WAYBAR_CSS constants, so it does not exercise the migration branch in patch_waybar_style. Add coverage around the patch_waybar_style function using an old CSS sample that would reveal the orphaned-line issue and verify the transformed CSS is preserved as expected.src/setup_gnome.rs (1)
5-9: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueDelete stale
icons/files on upgrade.install_extension()now writes the extension assets at the extension root, so older installs can keep an unusedicons/server-symbolic.svgunless you remove it during install.🤖 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/setup_gnome.rs` around lines 5 - 9, The install_extension flow leaves behind stale extension assets from older installs, specifically unused files under the icons directory. Update install_extension in setup_gnome.rs to remove the old icons/server-symbolic.svg path as part of the upgrade/install cleanup before or after writing the new extension assets, so the extension root stays in sync with the current files.
🤖 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 `@gnome-extension/port-killer`@local/extension.js:
- Around line 201-228: The loop timer in _syncLoopTimer is being stopped too
early when _setCount transitions to 0, which freezes the scramble animation
before the SCRAMBLE_MS window finishes. Update the shouldRun logic in
_syncLoopTimer (and, if needed, the _setCount transition that sets
_scrambleUntil) so the timer stays active while the current time is still before
_scrambleUntil, even when this._count is 0. Keep using _loopTimerId, _loopFrame,
and _matrix.queue_repaint() so the scramble can settle smoothly into the idle
state instead of waiting for the next refresh.
In `@gnome-extension/port-killer`@local/styles.js:
- Around line 48-61: The Breathe animation in `breathe()` has a phase that does
not line up with its `frameCount`, so the loop jumps at wraparound. Update the
sine phase in `breathe(frame)` so one or more full cycles complete exactly over
the 24-frame animation used in `ANIMATIONS` (for example by deriving the
multiplier from `frameCount`), keeping the first and last frames visually
continuous.
---
Outside diff comments:
In `@src/setup.rs`:
- Around line 341-355: The `patch_waybar_style` migration branch is only
filtering lines containing `custom-port-killer`, which leaves orphaned property
lines and closing braces behind when replacing an old `port-killer-server.svg`
style block. Update the removal logic in `setup.rs` so it removes the entire
legacy `#custom-port-killer` rule(s) using brace-depth or another block-aware
approach before appending `WAYBAR_CSS`, and keep the early return for
`port-killer-matrix-sprite.svg` unchanged. Add a regression test around
`patch_waybar_style` that feeds in a realistic multi-line old stylesheet and
verifies the rewritten output is valid rather than only matching constants.
---
Nitpick comments:
In `@src/main.rs`:
- Around line 230-248: The empty branch in the listener display logic duplicates
the zero-padding rule by hardcoding "00" instead of using the same count
formatting used in the non-empty path. Update the `main` status text
construction so the `count` value is rendered consistently with
`format!("{count:02}")` in both branches, keeping the `listeners.is_empty()`
handling and tooltip/class selection unchanged.
In `@src/setup_gnome.rs`:
- Around line 5-9: The install_extension flow leaves behind stale extension
assets from older installs, specifically unused files under the icons directory.
Update install_extension in setup_gnome.rs to remove the old
icons/server-symbolic.svg path as part of the upgrade/install cleanup before or
after writing the new extension assets, so the extension root stays in sync with
the current files.
In `@src/setup.rs`:
- Around line 401-407: Add a migration test for patch_waybar_style that feeds it
a realistic old-style Waybar CSS block and asserts the migrated output is
correct. The current waybar_css_includes_matrix_animation test only checks
WAYBAR_CSS constants, so it does not exercise the migration branch in
patch_waybar_style. Add coverage around the patch_waybar_style function using an
old CSS sample that would reveal the orphaned-line issue and verify the
transformed CSS is preserved as expected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b3aba9c-250f-48e6-9b9a-3aa4a48bca45
⛔ Files ignored due to path filters (1)
assets/icons/port-killer-matrix-sprite.svgis excluded by!**/*.svg
📒 Files selected for processing (10)
examples/waybar-port-killer.jsoncgnome-extension/port-killer@local/dot-matrix.jsgnome-extension/port-killer@local/extension.cssgnome-extension/port-killer@local/extension.jsgnome-extension/port-killer@local/metadata.jsongnome-extension/port-killer@local/styles.jssrc/icon.rssrc/main.rssrc/setup.rssrc/setup_gnome.rs
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.
Updated tray icon from static server icon to animated dot matrix that displays the current number of servers running. Added right-click functionality to cycle through animations and colours.
dot-matrix-demo.webm
Summary by CodeRabbit