Repository navigation
Stop copy-prompt tooltip jitter and show existing forks - #1311
Merged
Merged
Conversation
Replace remix/ui popovers on onboarding Copy prompt buttons with CSS tooltips that do not steal pointer events between stacked cards. Overlay viewerInstall when a signed-in user already has a matching kody_id or listing fork so onboarding, community search, and listing detail show Copy prompt / Installed (or Forked) instead of Install. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe PR resolves authenticated viewer install states for community listings, forks, and saved packages. It propagates these states through loaders and displays Installed, Forked, Copy prompt, and adaptation-required behavior across community and onboarding surfaces. ChangesViewer install overlays
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Viewer
participant CommunityLoader
participant InstallResolver
participant PackageAndForkRepositories
participant CommunitySurface
Viewer->>CommunityLoader: Request community listings or details
CommunityLoader->>InstallResolver: Resolve viewer install state
InstallResolver->>PackageAndForkRepositories: Query saved packages and forks
PackageAndForkRepositories-->>InstallResolver: Return matching records
InstallResolver-->>CommunityLoader: Return Installed, Forked, or adaptation-required state
CommunityLoader-->>CommunitySurface: Provide viewerInstall
CommunitySurface-->>Viewer: Render badges, status, and Copy prompt
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
kody-bot
marked this pull request as ready for review
August 8, 2026 06:05
Contributor
|
🔎 Preview deployed: https://kody-pr-1311.kody-a99.workers.dev Worker: Mocks:
|
Viewer-install overlay calls readAuthenticatedAppUser from public community and onboarding loaders, which previously assumed COOKIE_SECRET in tests that only exercised listing JSON. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
packages/worker/src/package-registry/repo.ts (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider sharing the binding-limit constant.
maxSqlBindingsPerChunkis also defined inpackages/worker/src/community/repo.ts. Two copies can drift if the D1 binding limit assumption changes. Move the constant next tochunkArrayin@kody-internal/sharedand import it in both modules.🤖 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 `@packages/worker/src/package-registry/repo.ts` at line 6, Move maxSqlBindingsPerChunk from package-registry/repo.ts and community/repo.ts into `@kody-internal/shared` alongside chunkArray, export it there, and update both repositories to import and use the shared constant while removing their local definitions.packages/worker/src/app/community-public.ts (1)
176-185: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the shared prompt bodies.
buildExistingInstallPromptduplicates the body ofbuildInstallSuccessPrompt(Line 166), andbuildExistingAdaptPromptduplicates the body ofbuildInstallAdaptPrompt(Line 173). Only the opening sentence differs. If the agent instructions change later, an author must edit four strings and can update only two. Extract the shared tail into a helper and pass the opening sentence.♻️ Example extraction
+function installSetupSteps(targetName: string) { + return `Call package_get for it and read its README, then walk me through any remaining setup: create required secrets or OAuth connections, approve package secret access if prompted, and run a quick test to confirm it works.` +} + export function buildExistingInstallPrompt(input: { targetName: string }) { - return `I already have the community package "${input.targetName}" in my Kody account. Call package_get for it and read its README, then walk me through any remaining setup: create required secrets or OAuth connections, approve package secret access if prompted, and run a quick test to confirm it works.` + return `I already have the community package "${input.targetName}" in my Kody account. ${installSetupSteps(input.targetName)}` }🤖 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 `@packages/worker/src/app/community-public.ts` around lines 176 - 185, Extract the shared instruction tails used by buildInstallSuccessPrompt and buildExistingInstallPrompt into a helper that accepts the differing opening sentence, and do the same for buildInstallAdaptPrompt and buildExistingAdaptPrompt. Update all four builders to reuse those helpers while preserving their current opening text and prompt behavior.packages/worker/src/community/viewer-install.node.test.ts (1)
29-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for the newest-fork fallback.
Every fork group in this test contains a fork whose
targetKodyIdmatches the listingkodyId. ThematchingKodyFork ?? listingForks[0]fallback inresolveViewerListingInstallsis therefore never exercised. Add a listing with two forks that both use a renamedtargetKodyId, then assert that the newest fork wins.🤖 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 `@packages/worker/src/community/viewer-install.node.test.ts` around lines 29 - 51, Extend the fixture data used by the viewer install test with a listing whose two forks have renamed targetKodyId values, so neither matches the listing kodyId. Add assertions covering resolveViewerListingInstalls to verify the fallback selects the newest fork rather than the older one.packages/worker/src/community/repo.ts (1)
738-756: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument that the returned order is per chunk only.
ORDER BY created_at ASCapplies inside each chunk. WhenlistingIdsspans more than one chunk, the concatenatedforksarray is not globally sorted. The current consumerresolveViewerListingInstallsre-sorts each listing group, so behavior is correct today. Add a short doc comment so a future caller does not rely on a global order.🤖 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 `@packages/worker/src/community/repo.ts` around lines 738 - 756, The function returning the concatenated forks must document that ordering is only guaranteed within each SQL chunk, not globally across all listing IDs. Add a short doc comment immediately before the relevant function or return flow around the chunked query, noting that callers requiring global ordering must sort the result themselves; do not change the query or consumer behavior.packages/worker/src/app/community-data.ts (1)
300-355: 🚀 Performance & Scalability | 🔵 TrivialConsider the added per-request database cost on the index page.
For a signed-in viewer, every community index request now runs three parallel D1 queries plus an optional fourth, with
listingIdsandkodyIdsscaling to thelimitcap of 100. The public listing rows still come from the data cache, but the overlay does not. Consider a short-lived per-user cache keyed by user id, or restricting the overlay to the listings actually rendered above the fold, if index traffic from signed-in users is high.🤖 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 `@packages/worker/src/app/community-data.ts` around lines 300 - 355, Reduce the per-request database load in loadViewerListingInstalls by avoiding overlay queries for every cached index listing. Prefer a short-lived per-user cache keyed by userId for the resolved viewer installs; alternatively, limit input.listings to the above-the-fold listings before building listingIds and kodyIds, while preserving the existing install-resolution behavior.packages/worker/src/app/community-data.node.test.ts (1)
124-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReset mocks in a shared hook and assert the scoped
userId.Two issues in this file:
- Only
listSavedPackagesByKodyIdsis reset, and only in two of the four tests. Every other mock keeps its value across tests, so the suite depends on execution order. Add abeforeEachthat callsvi.resetAllMocks()and re-applies defaults, or enableclearMocksin the Vitest config.- No test asserts that the repository lookups receive the signed-in viewer's
userId. Per-user isolation is enforced only by theuser_idfilter in the real queries, which are mocked here. Assert the argument so a future refactor cannot drop the scoping.As per coding guidelines: "Maintain complete per-user isolation: each signed-in user must have an independent assistant with separate packages, jobs, secrets, values, memories, remote connectors, email inboxes, and durable storage."
💚 Suggested additions
+beforeEach(() => { + vi.resetAllMocks() +}) + test('community index overlays matching kody_id installs for signed-in viewers', async () => {expect(data.listings[0]?.viewerInstall).toEqual({ status: 'installed', targetName: '`@burhan/github`', agentPrompt: buildExistingInstallPrompt({ targetName: '`@burhan/github`' }), }) + expect(mockModule.listSavedPackagesByKodyIds).toHaveBeenCalledWith( + undefined, + expect.objectContaining({ userId: 'viewer-1' }), + ) + expect(mockModule.listCommunityForksByListingIdsAndUser).toHaveBeenCalledWith( + undefined, + expect.objectContaining({ userId: 'viewer-1' }), + ) })Also applies to: 189-203
🤖 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 `@packages/worker/src/app/community-data.node.test.ts` around lines 124 - 133, Update the test setup around the community index tests to add a shared beforeEach that calls vi.resetAllMocks() and reapplies the required default mock responses, removing the ad hoc partial resets. In the signed-in viewer tests, assert that repository lookup mocks such as listSavedPackagesByKodyIds and listCommunityForksByListingIdsAndUser receive the authenticated user’s userId, preserving per-user isolation.Source: Coding guidelines
🤖 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 `@packages/worker/src/app/community-data.ts`:
- Around line 102-109: Wrap the readAuthenticatedAppUser call in
loadCommunityIndexData with a handler-level try/catch; log any session parsing
or user lookup failure and continue with user set to null. Keep the existing
listings flow and pass the resolved anonymous user into
overlayViewerInstallsOnListings so the public community index still loads.
---
Nitpick comments:
In `@packages/worker/src/app/community-data.node.test.ts`:
- Around line 124-133: Update the test setup around the community index tests to
add a shared beforeEach that calls vi.resetAllMocks() and reapplies the required
default mock responses, removing the ad hoc partial resets. In the signed-in
viewer tests, assert that repository lookup mocks such as
listSavedPackagesByKodyIds and listCommunityForksByListingIdsAndUser receive the
authenticated user’s userId, preserving per-user isolation.
In `@packages/worker/src/app/community-data.ts`:
- Around line 300-355: Reduce the per-request database load in
loadViewerListingInstalls by avoiding overlay queries for every cached index
listing. Prefer a short-lived per-user cache keyed by userId for the resolved
viewer installs; alternatively, limit input.listings to the above-the-fold
listings before building listingIds and kodyIds, while preserving the existing
install-resolution behavior.
In `@packages/worker/src/app/community-public.ts`:
- Around line 176-185: Extract the shared instruction tails used by
buildInstallSuccessPrompt and buildExistingInstallPrompt into a helper that
accepts the differing opening sentence, and do the same for
buildInstallAdaptPrompt and buildExistingAdaptPrompt. Update all four builders
to reuse those helpers while preserving their current opening text and prompt
behavior.
In `@packages/worker/src/community/repo.ts`:
- Around line 738-756: The function returning the concatenated forks must
document that ordering is only guaranteed within each SQL chunk, not globally
across all listing IDs. Add a short doc comment immediately before the relevant
function or return flow around the chunked query, noting that callers requiring
global ordering must sort the result themselves; do not change the query or
consumer behavior.
In `@packages/worker/src/community/viewer-install.node.test.ts`:
- Around line 29-51: Extend the fixture data used by the viewer install test
with a listing whose two forks have renamed targetKodyId values, so neither
matches the listing kodyId. Add assertions covering resolveViewerListingInstalls
to verify the fallback selects the newest fork rather than the older one.
In `@packages/worker/src/package-registry/repo.ts`:
- Line 6: Move maxSqlBindingsPerChunk from package-registry/repo.ts and
community/repo.ts into `@kody-internal/shared` alongside chunkArray, export it
there, and update both repositories to import and use the shared constant while
removing their local definitions.
🪄 Autofix
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 Plus
Run ID: 962ad244-9ceb-4749-ad4a-9408bcf67fb6
📒 Files selected for processing (21)
docs/contributing/community-packages.mddocs/use/community-packages.mdpackages/worker/client/routes/community-detail.tsxpackages/worker/client/routes/onboarding-diy-card.tsxpackages/worker/client/routes/onboarding-starter-card.tsxpackages/worker/src/app/community-data.node.test.tspackages/worker/src/app/community-data.tspackages/worker/src/app/community-detail-content.tsxpackages/worker/src/app/community-listings-content.tsxpackages/worker/src/app/community-public.tspackages/worker/src/app/handlers/community-detail.frame.node.test.tspackages/worker/src/app/handlers/community-detail.tsxpackages/worker/src/app/handlers/community.frame.node.test.tspackages/worker/src/app/handlers/community.node.test.tspackages/worker/src/app/handlers/onboarding.node.test.tspackages/worker/src/community/repo.tspackages/worker/src/community/viewer-install.node.test.tspackages/worker/src/community/viewer-install.tspackages/worker/src/package-registry/repo.tspackages/worker/universal/community-public-types.tspackages/worker/universal/loader-data.ts
Session parsing or user lookup errors on /community, onboarding featured listings, and listing detail now degrade to anonymous listings instead of failing the page. Overlay queries stay scoped to the signed-in user. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
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
3 participants
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.
Summary
Onboarding Copy prompt tooltips were jittering between vertically stacked starter cards (e.g.
@kody/githuband@kody/cloudflare). Remix UI popovers flip into nearby buttons and steal hover, so those tips now use CSS hover/focus tooltips withpointer-events: none.Signed-in viewers who already have a matching
kody_idsaved package or a fork of the listing now see Copy prompt / Installed (or Forked) instead of Install on onboarding, community search cards, and listing detail.Changes
remix/ui/popoveron onboarding Copy prompt buttons with CSS tooltips that cannot become hover targets.viewerInstallfrom saved packages (kody_id) andcommunity_forkswithout putting viewer state in the public listing cache./communitycards and listing detail, and prefill Copy prompt + agent prompt when a fork already exists.Tests
resolveViewerListingInstallsprefers matchingkody_id, then listing forks.System recap — extends existing primitives (medium risk)
Mode: recap · Base:
main@1916ffea· Head:1a2cdcdeClassification: extends — viewer-install overlay changes community listing payloads and UI; saved-package lookup gains batch kody_id/id helpers.
Primitives touched
app-uicommunity-listingssaved-packageslistSavedPackagesByKodyIds/listSavedPackagesByIdsSystem map
Signed-in community browse loads public listings, then overlays the viewer's existing fork/install so onboarding and listing UI show Copy prompt instead of Install.
Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).
Change flow
Before / after
Onboarding Copy prompt tooltip
Signed-in listing surfaces
Summary by CodeRabbit
New Features
Documentation
Tests