-
Notifications
You must be signed in to change notification settings - Fork 452
chore: adds tasks statusbar image to experimental feature #13567
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughImports a WebP asset (tasks.StatusBar.webp) into the tasks plugin and assigns it to the experimental StatusBar configuration by adding an image property. The test expectation in task-manager.spec.ts is updated to assert the presence of an image string containing ".webp". No other runtime logic or public API signatures are changed. Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @gastoner - I've reviewed your changes - here's some feedback:
- You imported tasks.StatusBar.gif but the actual asset isn’t included in this diff—make sure the gif file is committed under assets so the import resolves correctly.
- To avoid potential bundler or OS issues with multiple dots in the filename, consider using a hyphenated, lowercase name (e.g. tasks-status-bar.gif).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- You imported tasks.StatusBar.gif but the actual asset isn’t included in this diff—make sure the gif file is committed under assets so the import resolves correctly.
- To avoid potential bundler or OS issues with multiple dots in the filename, consider using a hyphenated, lowercase name (e.g. tasks-status-bar.gif).
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/main/src/plugin/tasks/task-manager.ts (1)
79-81
: Verify UI integration for experimental.image in the Tasks settingsI searched the codebase and found this is the only use of
experimental.image
(packages/main/src/plugin/tasks/task-manager.ts lines 78–81) and no existing UI code reads or renders it. Please:
- Confirm the Preferences UI schema and renderer include the
image
property for this experimental option.- If missing, update the UI settings schema (e.g. in your settings-schema file) to declare
experimental.image
, and modify the relevant React component to render it (e.g. an<img src={experimental.image} />
in the Tasks experimental panel).- Add a lightweight test:
- To assert the configuration registration (see packages/api/src/configuration/models.ts:
image?: string
) includes theimage
field.- To verify the UI actually displays the imported
tasksManagerImage
in the Tasks experimental setting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/main/src/assets/tasks.StatusBar.gif/tasks.StatusBar.gif
is excluded by!**/*.gif
📒 Files selected for processing (1)
packages/main/src/plugin/tasks/task-manager.ts
(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: unit tests / macos-15
- GitHub Check: unit tests / windows-2025
- GitHub Check: macOS
- GitHub Check: typecheck
- GitHub Check: linter, formatters
- GitHub Check: build website
- GitHub Check: Windows
- GitHub Check: Linux
b8616bb
to
a17b55d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
a17b55d
to
95c2ff2
Compare
95c2ff2
to
6279ed0
Compare
6279ed0
to
c0b4f05
Compare
Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz>
c0b4f05
to
9244da1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/main/src/plugin/tasks/task-manager.spec.ts (1)
105-112
: Synchronize asset format between code/tests and PR descriptionTests and code currently use a WebP (
tasks.StatusBar.webp
) asset, but the PR title/description mentions GIF. Please align these to avoid confusion:• In
packages/main/src/plugin/tasks/task-manager.ts
(around line 30), you importtasks.StatusBar.webp
.
• Inpackages/main/src/plugin/tasks/task-manager.spec.ts
(lines 105–112), the test assertsexpect.stringContaining('.webp')
.Options to resolve:
- Switch to a GIF asset: rename/import
tasks.StatusBar.gif
and update the spec to.gif
.- Keep WebP and update the PR title/description to reference WebP.
- Make the test extension-agnostic, e.g.:
image: expect.stringMatching(/\.(webp|gif)$/),
🧹 Nitpick comments (1)
packages/main/src/plugin/tasks/task-manager.spec.ts (1)
109-111
: Make the assertion resilient to asset format changesPR text mentions a GIF, while the code currently uses a WebP. Future changes (e.g., switching formats or adding hashes/queries) would break this test. Prefer a regex that accepts common formats.
Apply this diff:
- image: expect.stringContaining('.webp'), + // Accept common image formats to avoid brittleness if the asset format changes + image: expect.stringMatching(/\.(webp|gif|png|svg)(\?.*)?$/),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/main/src/plugin/tasks/task-manager.spec.ts
(1 hunks)packages/main/src/plugin/tasks/task-manager.ts
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/main/src/plugin/tasks/task-manager.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build website
- GitHub Check: typecheck
- GitHub Check: linter, formatters
- GitHub Check: unit tests / macos-15
- GitHub Check: macOS
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: unit tests / windows-2025
- GitHub Check: smoke e2e tests (production)
- GitHub Check: smoke e2e tests (development)
- GitHub Check: Windows
- GitHub Check: Linux
🔇 Additional comments (1)
packages/main/src/plugin/tasks/task-manager.spec.ts (1)
104-112
: LGTM: test now asserts an image in the experimental StatusBar configThis aligns with the runtime change that wires an image into the experimental StatusBar configuration.
What does this PR do?
Adds gif for statusbar providers experimental feature
Screenshot / video of UI
What issues does this PR fix or reference?
How to test this PR?