cleaned up build output w/ "debug" lines and filtering #2545
Conversation
WalkthroughThe diff introduces a new AppInfo type (appid, modtime) and changes multiple RPC and server/client signatures to return AppInfo slices instead of string slices. waveappstore gains GetAppModTime and populates AppInfo entries; frontend types and the app-selection modal are updated to use AppInfo and show relative “last updated” times with a new formatRelativeTime util. A CreateNewWaveApp subcomponent refactors the create flow. The build panel adds a showDebug toggle. Tsunami builder added BuildOpts.AppNS, threads AppNS into createGoMod, and converts many verbose logs to use a “[debug]” prefix. A few stray debug logs elsewhere were removed. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ 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: 1
🧹 Nitpick comments (4)
tsunami/build/build.go (2)
91-91: Consider adding documentation for the AppNS field.The new
AppNSfield lacks a comment explaining its purpose. Adding a brief comment would help clarify that it represents the app namespace used in module path construction.+ // AppNS is the app namespace used in module path construction (e.g., "tsunami/appNS/appName") AppNS string
804-829: Output filtering logic is clear but buffers all output in memory.The filtering successfully removes verbose Tailwind output (version info and timing), but
CombinedOutput()captures all output before processing. For typical Tailwind builds this is fine, but if output becomes large this could consume significant memory.If memory usage becomes a concern, consider streaming line-by-line processing instead of buffering all output.
frontend/util/util.ts (1)
455-473: Consider handling future timestamps.The function works well for past timestamps, but if
timestampis in the future, it will produce unexpected results (negative values or "0 mins ago").Consider adding a boundary check:
function formatRelativeTime(timestamp: number): string { const now = Date.now(); + if (timestamp > now) { + return new Date(timestamp).toLocaleDateString(); + } const diffInSeconds = Math.floor((now - timestamp) / 1000);This would gracefully handle future timestamps, though they may not be expected in your use case.
frontend/builder/builder-buildpanel.tsx (1)
103-103: Remove empty comment.The empty comment
{/* */}appears to be a placeholder or accidental leftover.- {/* */} {filteredLines.length === 0 ? (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
frontend/app/store/wshclientapi.ts(1 hunks)frontend/builder/app-selection-modal.tsx(6 hunks)frontend/builder/builder-buildpanel.tsx(4 hunks)frontend/builder/store/builder-buildpanel-model.ts(1 hunks)frontend/types/gotypes.d.ts(1 hunks)frontend/util/util.ts(2 hunks)pkg/buildercontroller/buildercontroller.go(4 hunks)pkg/waveappstore/waveappstore.go(3 hunks)pkg/wcore/workspace.go(0 hunks)pkg/wshrpc/wshclient/wshclient.go(1 hunks)pkg/wshrpc/wshrpctypes.go(2 hunks)pkg/wshrpc/wshserver/wshserver.go(1 hunks)pkg/wstore/wstore_dbops.go(0 hunks)tsunami/build/build.go(15 hunks)
💤 Files with no reviewable changes (2)
- pkg/wstore/wstore_dbops.go
- pkg/wcore/workspace.go
🧰 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/waveappstore/waveappstore.go
🧬 Code graph analysis (8)
frontend/types/gotypes.d.ts (1)
pkg/wshrpc/wshrpctypes.go (1)
AppInfo(959-962)
pkg/wshrpc/wshrpctypes.go (2)
frontend/app/store/wshclientapi.ts (1)
ListAllEditableAppsCommand(366-368)pkg/wshrpc/wshclient/wshclient.go (1)
ListAllEditableAppsCommand(444-447)
pkg/wshrpc/wshclient/wshclient.go (2)
frontend/app/store/wshclientapi.ts (1)
ListAllEditableAppsCommand(366-368)pkg/wshrpc/wshrpctypes.go (2)
RpcOpts(351-357)AppInfo(959-962)
pkg/buildercontroller/buildercontroller.go (1)
pkg/waveappstore/waveappstore.go (1)
ParseAppId(40-51)
pkg/wshrpc/wshserver/wshserver.go (3)
frontend/app/store/wshclientapi.ts (1)
ListAllEditableAppsCommand(366-368)pkg/wshrpc/wshclient/wshclient.go (1)
ListAllEditableAppsCommand(444-447)pkg/wshrpc/wshrpctypes.go (1)
AppInfo(959-962)
frontend/app/store/wshclientapi.ts (1)
pkg/wshrpc/wshrpctypes.go (2)
RpcOpts(351-357)AppInfo(959-962)
frontend/builder/app-selection-modal.tsx (4)
pkg/wshrpc/wshrpctypes.go (1)
AppInfo(959-962)frontend/app/store/wshclientapi.ts (1)
RpcApi(647-647)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)frontend/util/util.ts (1)
formatRelativeTime(487-487)
pkg/waveappstore/waveappstore.go (1)
pkg/wshrpc/wshrpctypes.go (1)
AppInfo(959-962)
⏰ 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). (3)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (18)
tsunami/build/build.go (8)
124-127: LGTM!The field alignment improves readability.
275-279: LGTM!The debug logging with
[debug]prefix is consistent and appropriately guarded by the verbose flag.
336-341: LGTM! Default namespace behavior is sensible.The default of "app" for empty
appNSprovides a reasonable fallback. The module path construction follows the expected patterntsunami/{appNS}/{appName}.
418-436: LGTM! Improved output handling with consistent error reporting.The explicit
Flush()call ensures captured output is properly written. The generic error message at line 431 is appropriate since the actual error details are captured in the output.
655-657: LGTM!The updated function call correctly passes
opts.AppNStocreateGoMod.
756-777: LGTM! Simplified build command and consistent output handling.Using
"."as the build context is cleaner than explicitly listing go files. The debug logging and output capture are consistent with the rest of the file.
316-326: LGTM!The debug logging for node path detection is consistent with the go path logging style.
1041-1041: LGTM!Debug logging for symlink operations is helpful for troubleshooting.
pkg/buildercontroller/buildercontroller.go (4)
23-23: LGTM!Import reordering improves organization.
233-233: LGTM!The
appNSis correctly passed toBuildOptsfor use in the build process.
246-253: LGTM! Improved error handling ensures output is captured.Moving the error check after iterating through output lines ensures that all build output is captured and published to the user before handling the error. This provides better visibility into build failures.
192-196: The code pattern is correct—no inconsistency issue.The ParseAppId and GetAppName calls serve complementary purposes, not redundant ones. ParseAppId validates the appId format (namespace/name) and extracts structured components, while GetAppDir uses those components to construct a filesystem path (.../waveapps/namespace/name), and GetAppName extracts the directory name back. The appName values remain consistent throughout: the name component from ParseAppId matches what GetAppName extracts from the constructed path. Early validation is good defensive programming—it fails fast on malformed appIds before proceeding with build operations.
frontend/builder/store/builder-buildpanel-model.ts (1)
15-15: LGTM! Debug toggle state added.The new
showDebugatom provides a clean way to control debug line visibility. The default value offalseappropriately hides debug output initially.pkg/wshrpc/wshrpctypes.go (2)
329-329: LGTM! API signature updated to return structured app info.The change from
[]stringto[]AppInfoprovides richer metadata including modification times, which enables better UX in the app list.
959-962: LGTM! AppInfo structure looks good.The struct properly defines app metadata with appropriate JSON tags. The
ModTimefield will enable displaying "last updated" timestamps in the UI.pkg/wshrpc/wshserver/wshserver.go (1)
951-953: LGTM! Server implementation updated to return AppInfo.The signature change correctly aligns with the interface update and delegates to the app store layer.
frontend/builder/builder-buildpanel.tsx (2)
76-80: LGTM! Debug filtering logic is well-implemented.The toggle mechanism and filtering logic correctly show/hide debug lines. The filteredLines computation efficiently filters lines starting with "[debug]" when the toggle is off.
84-95: LGTM! Debug toggle UI is clear and accessible.The checkbox is properly integrated into the header with clear labeling and state binding.
| <span className="text-[11px] text-muted mt-0.5"> | ||
| Last updated: {formatRelativeTime(appInfo.modtime)} | ||
| </span> |
There was a problem hiding this comment.
Guard against zero modtime before rendering.
GetAppModTime returns 0 when an app doesn’t have an app.go yet (fresh drafts, deleted entrypoints, etc.), so this prints “Last updated: 55 years ago”. Please treat modtime <= 0 as “Never” (or hide the line) before calling formatRelativeTime.
Apply this diff to handle the sentinel:
- <span className="text-[11px] text-muted mt-0.5">
- Last updated: {formatRelativeTime(appInfo.modtime)}
- </span>
+ <span className="text-[11px] text-muted mt-0.5">
+ Last updated:{" "}
+ {appInfo.modtime > 0 ? formatRelativeTime(appInfo.modtime) : "Never"}
+ </span>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <span className="text-[11px] text-muted mt-0.5"> | |
| Last updated: {formatRelativeTime(appInfo.modtime)} | |
| </span> | |
| <span className="text-[11px] text-muted mt-0.5"> | |
| Last updated:{" "} | |
| {appInfo.modtime > 0 ? formatRelativeTime(appInfo.modtime) : "Never"} | |
| </span> |
🤖 Prompt for AI Agents
In frontend/builder/app-selection-modal.tsx around lines 192 to 194, guard
against appInfo.modtime being the sentinel 0 (or negative) before calling
formatRelativeTime: check if appInfo.modtime <= 0 and in that case render "Last
updated: Never" (or omit the entire line) instead of calling formatRelativeTime;
otherwise call formatRelativeTime(appInfo.modtime) as before.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/builder/builder-buildpanel.tsx (2)
80-80: Consider memoizing the filtered lines for better performance.The
filteredLinescomputation runs on every render, which could impact performance with large build outputs. Wrapping it inuseMemowould ensure the filtering only occurs whenoutputLinesorshowDebugchanges.Apply this diff to memoize the computation:
+import { memo, useCallback, useEffect, useMemo, useRef } from "react"; -import { memo, useCallback, useEffect, useRef } from "react";- const filteredLines = showDebug ? outputLines : outputLines.filter((line) => !line.startsWith("[debug]")); + const filteredLines = useMemo( + () => (showDebug ? outputLines : outputLines.filter((line) => !line.startsWith("[debug]"))), + [showDebug, outputLines] + );
86-94: Optional: Enhance checkbox accessibility.While wrapping the checkbox in a label is acceptable, you could further improve accessibility by adding an explicit
idandhtmlForattribute.Apply this diff to enhance accessibility:
- <label className="flex items-center gap-2 text-sm text-gray-300 cursor-pointer"> + <label htmlFor="debug-toggle" className="flex items-center gap-2 text-sm text-gray-300 cursor-pointer"> <input + id="debug-toggle" type="checkbox" checked={showDebug} onChange={handleDebugToggle} className="cursor-pointer" /> Debug </label>pkg/waveappstore/waveappstore.go (1)
533-533: Consider logging errors from GetAppModTime.The error from
GetAppModTimeis silently discarded. If the function fails (e.g., due to the issue flagged in lines 464-467), apps may show a zero or epoch modification time without any indication that an error occurred. Consider logging the error for debugging purposes.Apply this diff to log the error:
- modTime, _ := GetAppModTime(modTimeAppId) + modTime, err := GetAppModTime(modTimeAppId) + if err != nil { + // Log error but continue with zero modTime + log.Printf("failed to get modtime for app %s: %v", modTimeAppId, err) + }Note: This requires importing the
logpackage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/builder/builder-buildpanel.tsx(4 hunks)frontend/util/util.ts(2 hunks)pkg/waveappstore/waveappstore.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/util/util.ts
🧰 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/waveappstore/waveappstore.go
🧬 Code graph analysis (1)
pkg/waveappstore/waveappstore.go (1)
pkg/wshrpc/wshrpctypes.go (1)
AppInfo(959-962)
⏰ 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). (3)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (1)
frontend/builder/builder-buildpanel.tsx (1)
103-108: LGTM!The JSX comment is a standard pattern to prevent unwanted whitespace in
<pre>tags, and the use offilteredLinesis correctly implemented throughout the rendering logic.
| dirInfo, err := os.Stat(appPath) | ||
| if err != nil { | ||
| return 0, nil | ||
| } |
There was a problem hiding this comment.
Return error when app directory doesn't exist.
When os.Stat(appPath) fails, the function returns (0, nil), silently indicating a zero modification time. This could be misleading, especially since ListAllEditableApps at line 533 ignores errors from this function. Consider returning the error or documenting that zero indicates a missing app.
Apply this diff to return the error:
dirInfo, err := os.Stat(appPath)
if err != nil {
- return 0, nil
+ return 0, err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dirInfo, err := os.Stat(appPath) | |
| if err != nil { | |
| return 0, nil | |
| } | |
| dirInfo, err := os.Stat(appPath) | |
| if err != nil { | |
| return 0, err | |
| } |
🤖 Prompt for AI Agents
In pkg/waveappstore/waveappstore.go around lines 464 to 467, the
os.Stat(appPath) error is currently swallowed by returning (0, nil); change this
to propagate the error by returning (0, err) (or wrap the error with context and
return that) so callers receive the real failure instead of a misleading zero
time.
No description provided.