podman-desktop icon indicating copy to clipboard operation
podman-desktop copied to clipboard

chore: added experimental feedback logic

Open gastoner opened this issue 8 months ago โ€ข 2 comments

What does this PR do?

Adds experimental feedback logic This PR adds peristent configuration to settings.json file: can look like this:

  "features.timestamp": {
    "kubernetes.statesExperimental": null,
    "statusbarProviders.showProviders": 1748321012505
  }

Where:

  1. I have disabled features tasks.Manager / tasks.Statusbar => should be not in settings
  2. kubernetes.statesExperimental": null means that user clicked in dont show again and does not want to be notified with this feature
  3. 1748321012505 - some timestamp when user should be notified - during the first startup/enabling the feature timestamp is set to 2 days from now (for ease of testing, is this reduced to minutes - TODO migrate to days). When this timestamp is lower than the current timestamp, the dialog is shown during sthe tart of an app (only ONE dialog should be visible) - TODO add a timeout mechanism to avoid poping instantly after startup

Screenshot / video of UI

What issues does this PR fix or reference?

Closes https://github.com/podman-desktop/podman-desktop/issues/10228

  • [x] Needs to be rebased after https://github.com/podman-desktop/podman-desktop/pull/11987
  • [x] Needs to be rebased after https://github.com/podman-desktop/podman-desktop/pull/11991
  • [x] Needs to be rebased after https://github.com/podman-desktop/podman-desktop/pull/12432
  • [ ] Needs to be rebased after https://github.com/podman-desktop/podman-desktop/pull/12677 and https://github.com/podman-desktop/podman-desktop/pull/12678

The whole feature can be tried out in https://github.com/podman-desktop/podman-desktop/pull/11598

How to test this PR?

  • [ ] Tests are covering the bug fix or the new feature

gastoner avatar Apr 02 '25 16:04 gastoner

built with Refined Cloudflare Pages Action

โšก Cloudflare Pages Deployment

Name Status Preview Last Commit
podman-desktop-website-pr โœ… Ready (View Log) Visit Preview 82e54a843991107ef276c086d92a4e91b9ded890

github-actions[bot] avatar May 13 '25 07:05 github-actions[bot]

Codecov Report

:x: Patch coverage is 98.85057% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...rc/plugin/experimental-feature-feedback-handler.ts 98.85% 1 Missing :warning:

:loudspeaker: Thoughts on this report? Let us know!

codecov[bot] avatar May 14 '25 08:05 codecov[bot]

[!NOTE]

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

๐Ÿ“ Walkthrough

Walkthrough

Adds a feedback dialog flow to ExperimentalFeatureFeedbackHandler: the constructor now accepts MessageBox and Telemetry; init registers a feedback configuration and calls showFeedbackDialog; showFeedbackDialog scans enabled experimental features with GitHub links and, when remindAt is due, shows a dialog offering Share feedback (opens external GitHub link), reminder options (1 or 2 days), or Don't show again (disables feature). Remind timestamps and disabled state are persisted and telemetry events are emitted. Tests were expanded to inject MessageBox and Telemetry and cover all dialog outcomes.

Estimated code review effort

๐ŸŽฏ 4 (Complex) | โฑ๏ธ ~45 minutes

Assessment against linked issues

Objective (issue#) Addressed Explanation
Implement feedback notification dialog for experimental features (#10228) โœ…
Trigger dialog for specific feature actions: New task manager, Docker compatibility, Statusbar Providers, Tasks in statusbar (#10228) โ“ No event listeners or wiring for the listed user actions (clear/hide task, enable/switch docker, pin/unpin provider, task click + delayed show) are present in this diff; only the dialog flow and timing are implemented.

Possibly related PRs

  • podman-desktop/podman-desktop#13567 โ€” Adds an image property to a feature config that the feedback dialog now consumes.
  • podman-desktop/podman-desktop#13317 โ€” Prior changes to ExperimentalFeatureFeedbackHandler; touches the same handler and timestamp/reminder logic.
  • podman-desktop/podman-desktop#13493 โ€” Related init/save logic changes affecting experimental feature config handling used by the new flow.

Suggested reviewers

  • benoitf
  • jeffmaury
  • cdrage

[!TIP]

๐Ÿ”Œ Remote MCP (Model Context Protocol) integration is now available!

Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats.

โœจ Finishing Touches
  • [ ] ๐Ÿ“ Generate Docstrings
๐Ÿงช Generate unit tests
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment

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.

โค๏ธ Share
๐Ÿชง Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

coderabbitai[bot] avatar Jun 30 '25 13:06 coderabbitai[bot]

@benoitf created https://github.com/podman-desktop/podman-desktop/pull/13049

gastoner avatar Jul 02 '25 15:07 gastoner

@benoitf Fixed

gastoner avatar Jul 04 '25 11:07 gastoner

I'm not feeling confident to merge it the same day of the release, as there won't any much usage of this.

Due to timestamps/ latetimers I have the feeling we may have forgotten some corner cases. And if it's become annoying for the user we would have multiple reports.

I think this is the kind of stuff that needs to be merged at the beginning of the release cycle, not close to the release date.

benoitf avatar Jul 04 '25 13:07 benoitf

I've noticed it's storing entries in my config file as features.timestamp but IMHO it is not self explanatory

like, what is this timestamp for ? last enablement time, last disabled time, ?

why is it called feature while it's for experimental stuff ?

and I'm finding more confusing that timestamp could be 'disabled'

I think the properties being stored should be reviewed as it won't scale

If we want to introduce new fields it won't be possible

also I would expect the timestamp would stay in all case (like when was the time where it has been disabled for example) ?

and why a new top level entry features.timestamp

We can merge this after release but I think this should be decided by @rujutashinde @Firewall @slemeur

Originaly I was thinking about having this as a 3 part like features.experimental, but I'm now not sure why I did not add the experimental in here. I can rename it to e.g. experimentalFeatures.notifyAt

And in this case I would change 'disabled' -> 'never' I'm not sure what would be best approach for storing the timestamps/never we need to catch 3 cases:

  1. store timestamp when we should show dialog
  2. how to store "don't show again" option
  3. how to store "disabled" experimental feature - I'm not sure if we want to keep the timestamp in here, we could and then check the feature if it is enabled. This would mean that if we select e.g. "tomorrow" and then disable the feature we wont get the dialog until we enable the feature again

gastoner avatar Jul 04 '25 13:07 gastoner

And in this case I would change 'disabled' -> 'never' I'm not sure what would be best approach for storing the timestamps/never we need to catch 3 cases:

store timestamp when we should show dialog how to store "don't show again" option how to store "disabled" experimental feature - I'm not sure if we want to keep the timestamp in here, we could and then check the feature if it is enabled. This would mean that if we select e.g. "tomorrow" and then disable the feature we wont get the dialog until we enable the feature again

to me there is something unclear there

if the experimental feature is disabled, why should I store a timestamp ? there is already a configuration setting for enabling/disabling the experimental feature. So it makes me feel that we're storing two things at different scopes ?

if we need to keep something maybe

{
experimentalFeatures:
    {
          disabled: ['feat1', 'feat2']
          remindAt: {
                 "feat1": 123456
          }
}

or something more scalable:

``json { "experimentalFeatures": { "feat1": { "enabled": false, "remindAt": 1234567890 }, "feat2": { "enabled": false }, "feat3": { "enabled": true } } }


it would also mean to merge/store experimental features under the same top level entry (because for now there is a top level entry for each experimental flag)

benoitf avatar Jul 04 '25 14:07 benoitf

@benoitf I've updated the persistent storage mechanism => renaming, added disabled array Not sure if we want to keep the remindAt when is the feature disabled from settings OR disabled by selecting "don't remind"?

gastoner avatar Jul 07 '25 07:07 gastoner

I got an idea for improvement:

We could add gifs for each feature

Screencast_20250811_141355.webm

cc @Firewall @vancura

gastoner avatar Aug 11 '25 12:08 gastoner

Excellent idea @gastoner! Normally I would be hesitant to add gifs to the product but since the nature of these is that they are temporary together with the experimental features, I think this is a great reminder of what the feature is that we are asking feedback on.

Firewall avatar Aug 11 '25 14:08 Firewall

This is so awesome! Great improvement! I am unsure about GIFs, though; they are not very well compressed. Considering we're in modern Electron, we can use a better format like WebM. This could also be automated via ffmpeg.

vancura avatar Aug 12 '25 08:08 vancura

@gastoner I see the code change for showing images, but no images are added yet - is that on purpose?

jiridostal avatar Aug 13 '25 12:08 jiridostal

FYI, we worked together on the video file format, and we'll use WebM here. It's much smaller than a GIF of the same resolution and framerate (and has more than 256 colors).

vancura avatar Aug 13 '25 13:08 vancura

@jiridostal yes this is expected, there should be visible either the thumbs up/down as a fallback or videos after

  1. https://github.com/podman-desktop/podman-desktop/pull/13562
  2. https://github.com/podman-desktop/podman-desktop/pull/13563
  3. https://github.com/podman-desktop/podman-desktop/pull/13567
  4. https://github.com/podman-desktop/podman-desktop/pull/13565 are merged (each PR is one feature video + relevant changes)

gastoner avatar Aug 13 '25 13:08 gastoner

@jiridostal @slemeur @Firewall I've added telemetry for this feature, so we can track on which option users clicks

gastoner avatar Aug 18 '25 06:08 gastoner