Skip to content

feat: Star on github popup in Rill developer - #9787

Open
k-anshul wants to merge 5 commits into
mainfrom
github_star
Open

feat: Star on github popup in Rill developer#9787
k-anshul wants to merge 5 commits into
mainfrom
github_star

Conversation

@k-anshul

@k-anshul k-anshul commented Aug 5, 2026

Copy link
Copy Markdown
Member
  • Nudges a user to star Rill's repo on dashboard load once every 24 hours.
  • If user clicks on star/don't show this again then nudge is never shown (unless user clears local memory).
image

Checklist:

  • Covered by tests
  • Ran it and it works as intended
  • Reviewed the diff before requesting a review
  • Checked for unhandled edge cases
  • Linked the issues it closes
  • Checked if the docs need to be updated. If so, create a separate Linear DOCS issue
  • Intend to cherry-pick into the release branch
  • I'm proud of this work!

@k-anshul k-anshul self-assigned this Aug 5, 2026
@k-anshul
k-anshul marked this pull request as ready for review August 5, 2026 06:59
Comment thread web-common/src/features/github-star/github-star.svelte.ts Outdated
Comment thread web-common/src/features/github-star/github-star.svelte.ts Outdated

@AdityaHegde AdityaHegde left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving with a couple of nits

),
) {}

public get state() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: do we need a getter here? Cant we just use this.store.value directly?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

store is private so can't be used in tests directly.

* Recomputed whenever the state changes; re-reading on each app load is ample
* granularity for the one-day mute to expire.
*/
public get visible() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: same here, we just add isVisible on this class that checks this.store.value

Or if we want reactivity on visible this might be better,

this.visible = $derived.by(() => {
  const now = ...
  if (this.store.value.status !== "armed") return false;
  return !this.store.value.mutedUntil || this.store.value.mutedUntil <= now;
})

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inlined it into the getter and dropped the standalone helper. Kept it as a getter rather than $derived: store.value is $state, so the getter already re-renders consumers on state changes, and $derived would cache the Date.now() comparison, so the one-day mute wouldn't expire until some unrelated state change invalidated it.

@nishantmonu51 nishantmonu51 added Type:Feature New feature request Size:L Large change: 500-1,999 lines labels Aug 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Size:L Large change: 500-1,999 lines Type:Feature New feature request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants