show apps icon in the widgets bar which shows a tsunami app launcher #2554
Conversation
WalkthroughThis PR introduces a new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/app/workspace/widgets.tsx (2)
79-85: AppsFloatingWindow behavior is solid; consider avoiding repeated fetches and clarifying onOpenChangeThe floating apps window is well‑structured: it uses Floating UI correctly, handles loading/error states, filters out
draft/apps, sorts by display name, and cleanly launches a tsunami block with the selectedappid.Two minor improvements you might consider:
- Avoid refetching on every open: The effect refetches apps each time
isOpenflips totrue. Since the local apps set is usually stable within a session, you could hoistapps/loadingtoWidgets(or a Jotai atom) and only refetch on an explicit refresh or when you detect changes. This would reduce RPC traffic and spinner flicker, especially in dev where this is used frequently.- Future‑proof
onOpenChange: PassingonClose(a zero‑arg callback that always closes) touseFloating({ open, onOpenChange })is fine with the current setup where onlyuseDismissdrives it and only ever closes. If you later adduseClickor other interactions that callonOpenChange(true), this callback would immediately close instead of syncing the open state. A slightly more explicit version likeonOpenChange={(open) => !open && onClose()}would make the intent clearer and guard future changes.These are polish suggestions; the current implementation should work as intended.
Also applies to: 87-203
433-439: Supercompact height heuristic doesn’t account for the dev apps button (optional)In
checkModeNeeded, thetotalWidgetsestimate used for deciding when to switch to"supercompact"still only counts regular widgets plus help widgets. The measurement block now also includes the dev apps button, so in very tight vertical layouts the actual content height can slightly exceed the heuristic and still end up in"compact", causing a bit more clipping than expected in dev builds.Not a blocker, but if you want the mode switch to match reality more closely, you could include the dev apps button in
totalWidgetswhenisDev()is true to keep the estimate aligned with what you render.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
frontend/app/store/wshclientapi.ts(1 hunks)frontend/app/workspace/widgets.tsx(7 hunks)frontend/types/gotypes.d.ts(1 hunks)pkg/waveappstore/waveappstore.go(6 hunks)pkg/waveapputil/waveapputil.go(2 hunks)pkg/wshrpc/wshclient/wshclient.go(1 hunks)pkg/wshrpc/wshrpctypes.go(3 hunks)pkg/wshrpc/wshserver/wshserver.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-22T01:28:41.417Z
Learnt from: esimkowitz
Repo: wavetermdev/waveterm PR: 1790
File: pkg/remote/fileshare/wshfs/wshfs.go:122-122
Timestamp: 2025-01-22T01:28:41.417Z
Learning: The RpcClient in pkg/remote/fileshare/wshfs/wshfs.go is initialized and handled downstream by either main-server or wshcmd-connserver, as documented in the package comment.
Applied to files:
pkg/wshrpc/wshclient/wshclient.gopkg/wshrpc/wshserver/wshserver.go
🧬 Code graph analysis (7)
frontend/app/store/wshclientapi.ts (1)
pkg/wshrpc/wshrpctypes.go (2)
RpcOpts(363-369)AppInfo(971-975)
pkg/wshrpc/wshclient/wshclient.go (3)
frontend/app/store/wshclientapi.ts (1)
ListAllAppsCommand(366-368)pkg/wshutil/wshrpc.go (1)
WshRpc(47-61)pkg/wshrpc/wshrpctypes.go (2)
RpcOpts(363-369)AppInfo(971-975)
pkg/wshrpc/wshserver/wshserver.go (4)
frontend/app/store/wshclientapi.ts (1)
ListAllAppsCommand(366-368)pkg/wshrpc/wshclient/wshclient.go (1)
ListAllAppsCommand(444-447)pkg/wshrpc/wshrpctypes.go (1)
AppInfo(971-975)pkg/waveappstore/waveappstore.go (1)
ListAllApps(458-511)
pkg/wshrpc/wshrpctypes.go (3)
frontend/app/store/wshclientapi.ts (1)
ListAllAppsCommand(366-368)pkg/wshrpc/wshclient/wshclient.go (1)
ListAllAppsCommand(444-447)tsunami/engine/clientimpl.go (1)
AppManifest(55-60)
frontend/types/gotypes.d.ts (2)
pkg/wshrpc/wshrpctypes.go (1)
AppManifest(1069-1074)tsunami/engine/clientimpl.go (1)
AppManifest(55-60)
pkg/waveappstore/waveappstore.go (1)
pkg/wshrpc/wshrpctypes.go (2)
AppInfo(971-975)AppManifest(1069-1074)
frontend/app/workspace/widgets.tsx (7)
pkg/wshrpc/wshrpctypes.go (1)
AppInfo(971-975)frontend/app/store/wshclientapi.ts (1)
RpcApi(677-677)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)pkg/waveobj/wtype.go (1)
BlockDef(244-247)frontend/app/store/global.ts (2)
createBlock(820-820)isDev(842-842)frontend/util/util.ts (1)
makeIconClass(500-500)frontend/app/element/tooltip.tsx (1)
Tooltip(143-176)
⏰ 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). (2)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (go)
🔇 Additional comments (12)
pkg/waveapputil/waveapputil.go (2)
7-7: LGTM!The
bytesimport is necessary to support the optimization on line 72.
72-72: Nice optimization!Using
bytes.NewReaderdirectly on the byte slice eliminates the unnecessary string conversion, improving both performance and code clarity.pkg/wshrpc/wshrpctypes.go (1)
159-187: ListAllApps wiring and manifest propagation look consistent
Command_ListAllApps,ListAllAppsCommandonWshRpcInterface, the expandedAppInfo(with optionalManifest), andAppManifestall line up cleanly with the waveappstore implementation and TS declarations. JSON tags are coherent and this remains backward‑compatible whenmanifestis absent.Also applies to: 334-356, 970-974, 1069-1074
frontend/types/gotypes.d.ts (1)
59-72: TS AppInfo extension matches Go structAdding
manifest?: AppManifestmirrors the GoAppInfoshape and stays backward‑compatible for callers that don’t expect it.pkg/wshrpc/wshserver/wshserver.go (1)
952-954: Server ListAllAppsCommand correctly delegates to waveappstoreThe new
ListAllAppsCommandmirrors existing builder RPC patterns and cleanly forwards towaveappstore.ListAllApps(), returning[]wshrpc.AppInfoand error as expected.frontend/app/store/wshclientapi.ts (1)
365-368: RpcApi ListAllAppsCommand matches backend contractThe new
ListAllAppsCommandcorrectly targets"listallapps"with a null payload and returnsPromise<AppInfo[]>, consistent with the Go client/server wiring.pkg/wshrpc/wshclient/wshclient.go (1)
443-447: Go wshclient ListAllAppsCommand is correctly wiredThe client stub follows the established pattern (
sendRpcRequestCallHelper[[]wshrpc.AppInfo](…, "listallapps", nil, opts)) and matches the server method and TS binding.frontend/app/workspace/widgets.tsx (1)
238-240: Apps launcher button integration into Widgets bar looks consistentThe new
isAppsOpenstate, sharedappsButtonRef, and dev‑only cube launcher are wired cleanly into both supercompact and normal/compact layouts:
- The button uses the existing widgets visual style and Tooltip patterns.
- Toggling
isAppsOpenis straightforward, andAppsFloatingWindowis guarded on bothisDev()andappsButtonRef.currentto avoid null references.- Clicking an app tiles to
createBlockwith the expected tsunami meta, then closes the window viaonClose.No issues from a correctness or UX perspective; the behavior is coherent with the rest of the widgets bar.
Also applies to: 345-366, 375-392, 410-416
pkg/waveappstore/waveappstore.go (4)
458-511: LGTM! Function correctly upgraded to return enriched AppInfo.The migration from returning
[]stringapp IDs to[]wshrpc.AppInfostructs is well-implemented. The function gracefully handles missing mod times and manifests by ignoring errors, which is appropriate for a listing operation where partial data is acceptable.
540-616: LGTM! Manifest priority logic is correct.The introduction of
manifestAppId(lines 588-599) correctly prioritizes the draft version for manifest reading when both local and draft versions exist. This ensures that the returned manifest reflects the most current editable state of the app.
804-842: LGTM! Parameter type updated consistently.The function signature correctly updated to accept
*wshrpc.AppManifest. The implementation remains unchanged and continues to work correctly with the new manifest type.
722-744: Migration from engine.AppManifest to wshrpc.AppManifest is complete and verified.All references to the old
engine.AppManifesttype have been successfully removed, andwshrpc.AppManifestis used consistently across the codebase. The type change inReadAppManifestis correct, and consumers of this function receive the expected type. No breaking changes detected.
No description provided.