Skip to content

Conversation

@jsantell
Copy link
Collaborator

@jsantell jsantell commented Dec 1, 2025

Summary by cubic

Moved favorite charm handling into a dedicated FavoriteButton component with optimistic local state and server sync. Simplifies the header and removes the toggle-favorite command from the command pipeline.

  • New Features

    • Added x-favorite-button that toggles favorites instantly, then syncs with the server; prefers local state until sync completes.
    • Star icon with tooltip; exported via components index.
  • Refactors

    • HeaderView now uses x-favorite-button and removes custom favorite listeners/state; small button-group style tweak.
    • Removed toggle-favorite command from RootView; processCommand is synchronous with try/catch.

Written for commit 3278376. Summary will update automatically on new commits.

@jsantell jsantell force-pushed the shell-is-favorites branch 2 times, most recently from 045c218 to cf12ab0 Compare December 1, 2025 18:09
@jsantell jsantell requested a review from bfollington December 1, 2025 18:32
Base automatically changed from shell-body-view to main December 1, 2025 18:40
@jsantell jsantell marked this pull request as ready for review December 1, 2025 18:41
@jsantell jsantell force-pushed the shell-is-favorites branch 2 times, most recently from 7b42618 to fbfffea Compare December 1, 2025 18:50
@jsantell jsantell force-pushed the shell-is-favorites branch 2 times, most recently from 67e2dfb to d6e959c Compare December 1, 2025 19:01
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

1 issue found across 5 files

Prompt for AI agents (all 1 issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="packages/shell/src/components/FavoriteButton.ts">

<violation number="1" location="packages/shell/src/components/FavoriteButton.ts:53">
P2: Ensure the sync task runs even when the favorite mutation fails so the optimistic state doesn’t stay stuck after an error.</violation>
</file>

Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR

Comment on lines +53 to +60
this.isFavoriteSync.run();
}

protected override willUpdate(changedProperties: PropertyValues): void {
if (changedProperties.has("charmId")) {
this.isFavorite = undefined;
}
}
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot Dec 1, 2025

Choose a reason for hiding this comment

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

P2: Ensure the sync task runs even when the favorite mutation fails so the optimistic state doesn’t stay stuck after an error.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/shell/src/components/FavoriteButton.ts, line 53:

<comment>Ensure the sync task runs even when the favorite mutation fails so the optimistic state doesn’t stay stuck after an error.</comment>

<file context>
@@ -0,0 +1,127 @@
+      await manager.addFavorite(charmCell);
+    }
+
+    this.isFavoriteSync.run();
+  }
+
</file context>
Suggested change
this.isFavoriteSync.run();
}
protected override willUpdate(changedProperties: PropertyValues): void {
if (changedProperties.has("charmId")) {
this.isFavorite = undefined;
}
}
try {
const charmCell = (await this.rt.cc().get(this.charmId, true)).getCell();
if (isFavorite) {
await manager.removeFavorite(charmCell);
} else {
await manager.addFavorite(charmCell);
}
} finally {
this.isFavoriteSync.run();
}
Fix with Cubic

Copy link
Contributor

@bfollington bfollington left a comment

Choose a reason for hiding this comment

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

Nice, definitely better to have this all bundled in one place. LGTM.

@jsantell jsantell merged commit d97f5f0 into main Dec 2, 2025
9 checks passed
@jsantell jsantell deleted the shell-is-favorites branch December 2, 2025 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants