Block Level Indicators/Badges, Update TabBar Styling, Add Badges/Flags to Tabs#3009
Merged
Block Level Indicators/Badges, Update TabBar Styling, Add Badges/Flags to Tabs#3009
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
WalkthroughThis PR replaces the tab-indicator subsystem with a unified badge system across backend, RPC, types, and frontend. It adds Badge and BadgeEvent types, a transient BadgeStore and RPC handlers (including a PID watcher and watch command), a new Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| ORef: *oref, | ||
| BadgeId: eventData.Badge.BadgeId, | ||
| } | ||
| err = wshclient.BadgeWatchPidCommand(RpcClient, watchData, &wshrpc.RpcOpts{Route: connRoute}) |
Contributor
There was a problem hiding this comment.
WARNING: This command calls BadgeWatchPidCommand which is only implemented in wshremote (remote server), not in wshserver (local server). When running wsh badge --pid locally without an SSH connection, this will fail with a "method not found" error because there's no handler in the local server. The BadgeWatchPidCommand handler needs to be added to pkg/wshrpc/wshserver/wshserver.go for local PID watching to work.
Contributor
Code Review SummaryStatus: 1 Critical Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (35 files)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/app/store/keymodel.ts (1)
68-72:⚠️ Potential issue | 🔴 CriticalReturn type annotation is incorrect; function can return
undefined.The function returns
focusedNode.data?.blockIdwhich can beundefined, but the signature declares: string. This mismatch can lead to runtime errors in callers that trust the type.Callers at lines 638, 692, and 704 access
bcmproperties without null checks:const bcm = getBlockComponentModel(getFocusedBlockInStaticTab()); if (bcm.openSwitchConnection != null) { ... } // NPE if bcm is undefined🐛 Proposed fix
-function getFocusedBlockInStaticTab(): string { +function getFocusedBlockInStaticTab(): string | undefined { const layoutModel = getLayoutModelForStaticTab(); const focusedNode = globalStore.get(layoutModel.focusedNode); return focusedNode?.data?.blockId; }Then update callers to handle
undefined:globalKeyMap.set("Cmd:g", () => { - const bcm = getBlockComponentModel(getFocusedBlockInStaticTab()); - if (bcm.openSwitchConnection != null) { + const blockId = getFocusedBlockInStaticTab(); + if (!blockId) return false; + const bcm = getBlockComponentModel(blockId); + if (bcm?.openSwitchConnection != null) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/store/keymodel.ts` around lines 68 - 72, The function getFocusedBlockInStaticTab currently declares a return type of string but returns focusedNode.data?.blockId which may be undefined; change the signature of getFocusedBlockInStaticTab to return string | undefined and update all callers (e.g., sites that call getBlockComponentModel(getFocusedBlockInStaticTab()) and then use bcm without checks) to handle the undefined case — either guard early (if (!id) return/skip) or null-check before accessing bcm properties like openSwitchConnection so you never assume non-null values from getFocusedBlockInStaticTab or getBlockComponentModel.
🧹 Nitpick comments (8)
package.json (1)
85-85: Remove@types/uuidfrom runtime dependencies.
uuidalready ships its own TypeScript declarations, so keeping@types/uuidhere is redundant, and putting it independenciesships a dev-only package in production. Please drop it, or at minimum move it todevDependencies.Also applies to: 145-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 85, The package.json currently lists "@types/uuid": "^10.0.0" as a runtime dependency; remove that entry (or move it to devDependencies) because the uuid package includes its own TypeScript types and `@types/uuid` should not be shipped in production; update the dependencies block to delete the "@types/uuid" line (or add it under devDependencies if you need it for local tooling) and run a quick npm/yarn install to verify the lockfile updates.pkg/baseds/baseds.go (1)
24-29: MakeBadgeEventactions mutually exclusive.
Clear,ClearAll,ClearById, andBadgecan all be set at once with this shape, so malformed events have no explicit precedence rule. A tagged-union style payload or a small validation helper would make this shared contract much harder to misuse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/baseds/baseds.go` around lines 24 - 29, BadgeEvent currently allows multiple action fields to be set at once; add a validation helper (e.g., func (e *BadgeEvent) Validate() error or ValidateBadgeEvent(e *BadgeEvent) error) that enforces a tagged-union rule: exactly one of Clear, ClearAll, ClearById (non-empty string), or Badge (non-nil) must be set, returning a clear error when zero or >1 are set; call this validator wherever events are unmarshaled or processed so malformed payloads are rejected early and make BadgeEvent's contract explicit.eslint.config.js (1)
79-81: Scope theget/eexemptions more narrowly.These patterns suppress unused-variable warnings for any parameter named
getand any local/caught variable namedeacross the repo, which will hide real dead-code regressions outside the intended Jotai/error-handler cases. Prefer_get/_e, or move this override to the specific files that need it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eslint.config.js` around lines 79 - 81, The current global ignore regexes (argsIgnorePattern, varsIgnorePattern, caughtErrorsIgnorePattern) are too broad—narrow them to only match underscored placeholders (e.g., change matches to require a leading underscore like `_get` and `_e`) or remove the global `get`/`e` exemptions and relocate the relaxed rules to the specific files or overrides that need them; update the regexes for argsIgnorePattern, varsIgnorePattern and caughtErrorsIgnorePattern accordingly or add an ESLint override block scoped to the specific file patterns that require these exceptions.pkg/util/unixutil/unixutil_unix.go (1)
72-78: Consider handlingEPERMfor processes owned by other users.The current implementation returns
falsewhensyscall.Kill(pid, 0)returns an error, butEPERMindicates the process exists but the caller lacks permission. For the badge watch use case (monitoring a PID the caller started), this works correctly since the caller owns the process.If broader use is anticipated, consider:
🔧 Optional fix to handle EPERM
func IsPidRunning(pid int) bool { if pid <= 0 { return false } err := syscall.Kill(pid, 0) - return err == nil + return err == nil || err == syscall.EPERM }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/util/unixutil/unixutil_unix.go` around lines 72 - 78, The IsPidRunning function currently treats any error from syscall.Kill(pid, 0) as “not running”; update it to treat EPERM as success (process exists but not permitted) by checking the returned error against syscall.EPERM (or errors.Is(err, syscall.EPERM)) and returning true in that case, while still returning false for other errors; adjust IsPidRunning to call syscall.Kill(pid, 0), then if err == nil return true, else if the error is EPERM return true, otherwise return false.frontend/app/tab/tabbar-model.ts (1)
4-15: Consider removing the empty singleton class.
TabBarModelis now a hollow singleton with no state or methods beyond the pattern itself, and it is not used anywhere in the codebase. If no longer needed, removing it would eliminate dead code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/tab/tabbar-model.ts` around lines 4 - 15, TabBarModel is an empty, unused singleton (class TabBarModel with private static instance and getInstance) that constitutes dead code; remove the entire TabBarModel class declaration and any associated references (instance, getInstance) from the codebase, or if it must be preserved for future use, add meaningful state/methods and use it where intended—preferably delete the class to eliminate dead code and run the test suite/TS compiler to ensure no imports/reference errors remain.frontend/preview/previews/tab.preview.tsx (1)
24-28: Duplicate badge IDs in preview data.Lines 26-27 both use
badgeid: "b1"for different badges within the same tab. While this is preview/demo data, using unique IDs would better represent production behavior and avoid potential confusion when debugging.Suggested fix
badges: [ { badgeid: "b2", icon: "circle-check", color: "#4ade80", priority: 3 }, - { badgeid: "b1", icon: "circle-small", color: "#fbbf24", priority: 1 }, - { badgeid: "b1", icon: "circle-small", color: "red", priority: 1 }, + { badgeid: "b1a", icon: "circle-small", color: "#fbbf24", priority: 1 }, + { badgeid: "b1b", icon: "circle-small", color: "red", priority: 1 }, ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/preview/previews/tab.preview.tsx` around lines 24 - 28, The preview data for badges contains duplicate badge IDs ("b1") in the badges array inside tab.preview (badges: [...] entries); update the badgeid values so each object in the badges array has a unique identifier (e.g., change the second "b1" to a new unique id) to mirror production behavior and avoid ID collisions when rendering or debugging the preview.frontend/app/store/badge.ts (1)
194-210: Different secondary sort directions betweensortBadgesandsortBadgesForTab.
sortBadgessorts by badgeid descending (b.badgeid < a.badgeid), whilesortBadgesForTabsorts ascending (a.badgeid < b.badgeid). If this is intentional (e.g., newest first for blocks, oldest first for tabs), consider adding a comment explaining the distinction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/store/badge.ts` around lines 194 - 210, The two comparators in sortBadges and sortBadgesForTab disagree on the secondary sort direction (badgeid): sortBadges currently orders badgeid descending while sortBadgesForTab orders ascending; decide whether they should match or the difference is intentional and then implement that change: either make both functions use the same comparator logic for badgeid (update sortBadges or sortBadgesForTab to use the same a.badgeid < b.badgeid ? -1 : ... pattern) or, if the asymmetry is intentional, add a short clarifying comment above sortBadges and sortBadgesForTab explaining the rationale (e.g., "blocks: newest first vs tabs: oldest first") so future readers understand the differing secondary sort directions.frontend/app/app.tsx (1)
253-268: Tab badge clearing lacks staleness check.Unlike the block clearing logic (lines 245-248), the tab clearing effect doesn't verify the tab is still focused before clearing. Consider adding a similar check.
Suggested improvement
const timeoutId = setTimeout(() => { if (!document.hasFocus()) { return; } + const currentTabId = globalStore.get(atoms.staticTabId); + if (currentTabId !== tabId) { + return; + } clearBadgesForTabOnFocus(tabId); }, delay);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/app.tsx` around lines 253 - 268, The tab-clear effect currently clears badges unconditionally after the timeout; add a staleness/focus check before calling clearBadgesForTabOnFocus(tabId): after the delay and before invoking clearBadgesForTabOnFocus, re-check document.hasFocus() and verify the tabId still corresponds to the active/focused tab (use the same focused-tab check or ref used by the block-clearing logic, e.g., compare against the current focusedTabId or ref the codebase uses) and only call clearBadgesForTabOnFocus(tabId) when both checks pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/util/unixutil/unixutil_windows.go`:
- Around line 44-46: The current Windows stub IsPidRunning returns false and
causes badges to clear; replace the stub with a real PID check that opens the
process handle for PROCESS_QUERY_LIMITED_INFORMATION (or
PROCESS_QUERY_INFORMATION) and queries its exit status (e.g., using
WaitForSingleObject with zero timeout or GetExitCodeProcess) to determine if the
process is still active; implement this in IsPidRunning using the Windows
syscall package or golang.org/x/sys/windows, ensure you close the handle, and
return true only when the process is alive and false otherwise (preserve
signature IsPidRunning(pid int) bool).
In `@pkg/wcore/badge.go`:
- Around line 31-41: InitBadgeStore currently ignores the error returned by
wshclient.EventSubCommand, so subscription failures are swallowed; modify
InitBadgeStore to capture the error from EventSubCommand (e.g., err :=
wshclient.EventSubCommand(...)) and return that error (or wrap it with context)
instead of discarding it so callers of InitBadgeStore can detect and handle
subscription failures; ensure the function signature remains InitBadgeStore()
error and that any existing rpcClient.EventListener.On registration remains
intact.
---
Outside diff comments:
In `@frontend/app/store/keymodel.ts`:
- Around line 68-72: The function getFocusedBlockInStaticTab currently declares
a return type of string but returns focusedNode.data?.blockId which may be
undefined; change the signature of getFocusedBlockInStaticTab to return string |
undefined and update all callers (e.g., sites that call
getBlockComponentModel(getFocusedBlockInStaticTab()) and then use bcm without
checks) to handle the undefined case — either guard early (if (!id) return/skip)
or null-check before accessing bcm properties like openSwitchConnection so you
never assume non-null values from getFocusedBlockInStaticTab or
getBlockComponentModel.
---
Nitpick comments:
In `@eslint.config.js`:
- Around line 79-81: The current global ignore regexes (argsIgnorePattern,
varsIgnorePattern, caughtErrorsIgnorePattern) are too broad—narrow them to only
match underscored placeholders (e.g., change matches to require a leading
underscore like `_get` and `_e`) or remove the global `get`/`e` exemptions and
relocate the relaxed rules to the specific files or overrides that need them;
update the regexes for argsIgnorePattern, varsIgnorePattern and
caughtErrorsIgnorePattern accordingly or add an ESLint override block scoped to
the specific file patterns that require these exceptions.
In `@frontend/app/app.tsx`:
- Around line 253-268: The tab-clear effect currently clears badges
unconditionally after the timeout; add a staleness/focus check before calling
clearBadgesForTabOnFocus(tabId): after the delay and before invoking
clearBadgesForTabOnFocus, re-check document.hasFocus() and verify the tabId
still corresponds to the active/focused tab (use the same focused-tab check or
ref used by the block-clearing logic, e.g., compare against the current
focusedTabId or ref the codebase uses) and only call
clearBadgesForTabOnFocus(tabId) when both checks pass.
In `@frontend/app/store/badge.ts`:
- Around line 194-210: The two comparators in sortBadges and sortBadgesForTab
disagree on the secondary sort direction (badgeid): sortBadges currently orders
badgeid descending while sortBadgesForTab orders ascending; decide whether they
should match or the difference is intentional and then implement that change:
either make both functions use the same comparator logic for badgeid (update
sortBadges or sortBadgesForTab to use the same a.badgeid < b.badgeid ? -1 : ...
pattern) or, if the asymmetry is intentional, add a short clarifying comment
above sortBadges and sortBadgesForTab explaining the rationale (e.g., "blocks:
newest first vs tabs: oldest first") so future readers understand the differing
secondary sort directions.
In `@frontend/app/tab/tabbar-model.ts`:
- Around line 4-15: TabBarModel is an empty, unused singleton (class TabBarModel
with private static instance and getInstance) that constitutes dead code; remove
the entire TabBarModel class declaration and any associated references
(instance, getInstance) from the codebase, or if it must be preserved for future
use, add meaningful state/methods and use it where intended—preferably delete
the class to eliminate dead code and run the test suite/TS compiler to ensure no
imports/reference errors remain.
In `@frontend/preview/previews/tab.preview.tsx`:
- Around line 24-28: The preview data for badges contains duplicate badge IDs
("b1") in the badges array inside tab.preview (badges: [...] entries); update
the badgeid values so each object in the badges array has a unique identifier
(e.g., change the second "b1" to a new unique id) to mirror production behavior
and avoid ID collisions when rendering or debugging the preview.
In `@package.json`:
- Line 85: The package.json currently lists "@types/uuid": "^10.0.0" as a
runtime dependency; remove that entry (or move it to devDependencies) because
the uuid package includes its own TypeScript types and `@types/uuid` should not be
shipped in production; update the dependencies block to delete the "@types/uuid"
line (or add it under devDependencies if you need it for local tooling) and run
a quick npm/yarn install to verify the lockfile updates.
In `@pkg/baseds/baseds.go`:
- Around line 24-29: BadgeEvent currently allows multiple action fields to be
set at once; add a validation helper (e.g., func (e *BadgeEvent) Validate()
error or ValidateBadgeEvent(e *BadgeEvent) error) that enforces a tagged-union
rule: exactly one of Clear, ClearAll, ClearById (non-empty string), or Badge
(non-nil) must be set, returning a clear error when zero or >1 are set; call
this validator wherever events are unmarshaled or processed so malformed
payloads are rejected early and make BadgeEvent's contract explicit.
In `@pkg/util/unixutil/unixutil_unix.go`:
- Around line 72-78: The IsPidRunning function currently treats any error from
syscall.Kill(pid, 0) as “not running”; update it to treat EPERM as success
(process exists but not permitted) by checking the returned error against
syscall.EPERM (or errors.Is(err, syscall.EPERM)) and returning true in that
case, while still returning false for other errors; adjust IsPidRunning to call
syscall.Kill(pid, 0), then if err == nil return true, else if the error is EPERM
return true, otherwise return false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 46d4f30a-03bd-47a4-aebd-69b4caf86468
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (44)
.roo/rules/rules.mdcmd/generatego/main-generatego.gocmd/server/main-server.gocmd/wsh/cmd/wshcmd-badge.gocmd/wsh/cmd/wshcmd-tabindicator.goemain/emain-menu.tseslint.config.jsfrontend/app/app.tsxfrontend/app/block/blockframe-header.tsxfrontend/app/store/badge.tsfrontend/app/store/global-atoms.tsfrontend/app/store/global.tsfrontend/app/store/keymodel.tsfrontend/app/store/wshclientapi.tsfrontend/app/tab/tab.scssfrontend/app/tab/tab.tsxfrontend/app/tab/tabbar-model.tsfrontend/app/tab/tabbar.scssfrontend/app/tab/tabbar.tsxfrontend/app/tab/vtab.tsxfrontend/app/tab/workspaceswitcher.scssfrontend/app/view/term/termwrap.tsfrontend/preview/previews/tab.preview.tsxfrontend/preview/previews/vtabbar.preview.tsxfrontend/tailwindsetup.cssfrontend/types/gotypes.d.tsfrontend/types/waveevent.d.tsfrontend/wave.tspackage.jsonpkg/baseds/baseds.gopkg/tsgen/tsgen.gopkg/tsgen/tsgenevent.gopkg/util/unixutil/unixutil_unix.gopkg/util/unixutil/unixutil_windows.gopkg/waveobj/metaconsts.gopkg/waveobj/wtypemeta.gopkg/wcore/badge.gopkg/wcore/tabindicator.gopkg/wps/wpstypes.gopkg/wshrpc/wshclient/wshclient.gopkg/wshrpc/wshremote/wshremote.gopkg/wshrpc/wshrpctypes.gopkg/wshrpc/wshserver/wshserver.goschema/waveai.json
💤 Files with no reviewable changes (2)
- pkg/wcore/tabindicator.go
- pkg/tsgen/tsgen.go
Comment on lines
+44
to
+46
| func IsPidRunning(pid int) bool { | ||
| return false | ||
| } |
Contributor
There was a problem hiding this comment.
Windows implementation will cause badges to clear immediately.
Returning false unconditionally means any wsh badge --pid command on Windows will immediately detect the process as "not running" and publish a badge clear event. This differs from Unix behavior where the badge persists while the process runs.
Consider implementing proper Windows PID checking:
🔧 Proposed fix using Windows process handle
+import (
+ "fmt"
+ "os"
+
+ "golang.org/x/sys/windows"
+)
-func IsPidRunning(pid int) bool {
- return false
-}
+func IsPidRunning(pid int) bool {
+ if pid <= 0 {
+ return false
+ }
+ handle, err := windows.OpenProcess(windows.PROCESS_QUERY_LIMITED_INFORMATION, false, uint32(pid))
+ if err != nil {
+ return false
+ }
+ windows.CloseHandle(handle)
+ return true
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/util/unixutil/unixutil_windows.go` around lines 44 - 46, The current
Windows stub IsPidRunning returns false and causes badges to clear; replace the
stub with a real PID check that opens the process handle for
PROCESS_QUERY_LIMITED_INFORMATION (or PROCESS_QUERY_INFORMATION) and queries its
exit status (e.g., using WaitForSingleObject with zero timeout or
GetExitCodeProcess) to determine if the process is still active; implement this
in IsPidRunning using the Windows syscall package or golang.org/x/sys/windows,
ensure you close the handle, and return true only when the process is alive and
false otherwise (preserve signature IsPidRunning(pid int) bool).
Comment on lines
+31
to
+41
| func InitBadgeStore() error { | ||
| log.Printf("initializing badge store\n") | ||
|
|
||
| rpcClient := wshclient.GetBareRpcClient() | ||
| rpcClient.EventListener.On(wps.Event_Badge, handleBadgeEvent) | ||
| wshclient.EventSubCommand(rpcClient, wps.SubscriptionRequest{ | ||
| Event: wps.Event_Badge, | ||
| AllScopes: true, | ||
| }, nil) | ||
|
|
||
| return nil |
Contributor
There was a problem hiding this comment.
Bubble up badge subscription failures.
Line 36 ignores the EventSubCommand error, so the new startup check in cmd/server/main-server.go can never fire and the server can come up with a non-functional badge store.
Suggested fix
func InitBadgeStore() error {
log.Printf("initializing badge store\n")
rpcClient := wshclient.GetBareRpcClient()
rpcClient.EventListener.On(wps.Event_Badge, handleBadgeEvent)
- wshclient.EventSubCommand(rpcClient, wps.SubscriptionRequest{
+ if err := wshclient.EventSubCommand(rpcClient, wps.SubscriptionRequest{
Event: wps.Event_Badge,
AllScopes: true,
- }, nil)
+ }, nil); err != nil {
+ return err
+ }
return nil
}📝 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.
Suggested change
| func InitBadgeStore() error { | |
| log.Printf("initializing badge store\n") | |
| rpcClient := wshclient.GetBareRpcClient() | |
| rpcClient.EventListener.On(wps.Event_Badge, handleBadgeEvent) | |
| wshclient.EventSubCommand(rpcClient, wps.SubscriptionRequest{ | |
| Event: wps.Event_Badge, | |
| AllScopes: true, | |
| }, nil) | |
| return nil | |
| func InitBadgeStore() error { | |
| log.Printf("initializing badge store\n") | |
| rpcClient := wshclient.GetBareRpcClient() | |
| rpcClient.EventListener.On(wps.Event_Badge, handleBadgeEvent) | |
| if err := wshclient.EventSubCommand(rpcClient, wps.SubscriptionRequest{ | |
| Event: wps.Event_Badge, | |
| AllScopes: true, | |
| }, nil); err != nil { | |
| return err | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/wcore/badge.go` around lines 31 - 41, InitBadgeStore currently ignores
the error returned by wshclient.EventSubCommand, so subscription failures are
swallowed; modify InitBadgeStore to capture the error from EventSubCommand
(e.g., err := wshclient.EventSubCommand(...)) and return that error (or wrap it
with context) instead of discarding it so callers of InitBadgeStore can detect
and handle subscription failures; ensure the function signature remains
InitBadgeStore() error and that any existing rpcClient.EventListener.On
registration remains intact.
Deploying waveterm with
|
| Latest commit: |
64f6d4f
|
| Status: | ⚡️ Build in progress... |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/preview/previews/tab.preview.tsx`:
- Around line 47-57: The tab object with tabId "preview-tab-5" has tabName "3
Badges" but the badges array contains four entries (badgeid
"b1","b2","b3","b4"); update tabName to "4 Badges" (or programmatically derive
the name from badges.length) so tabName accurately reflects the number of badges
in the badges array.
In `@pkg/wshrpc/wshremote/wshremote.go`:
- Around line 157-159: The watcher in wshremote.go calls
unixutil.IsPidRunning(data.Pid) and assumes it returns true while the process
exists; verify all OS-specific implementations under pkg/util/unixutil
(especially unixutil_windows.go) adhere to that contract and, if windows
currently returns false unconditionally, either implement a correct Windows
IsPidRunning that checks process existence or add an OS guard in the
BadgeWatchPidCommand registration/path to avoid enabling the watcher on Windows;
locate and update the unixutil.IsPidRunning implementations or the
BadgeWatchPidCommand call site in wshremote.go accordingly so behavior is
consistent across platforms.
- Around line 151-173: The goroutine started in BadgeWatchPidCommand should stop
when the provided ctx is canceled: change the for loop to select on ctx.Done()
and a timer/ticker (e.g., time.After or time.Ticker) instead of unconditionally
sleeping; when ctx.Done() fires return immediately and do not call
wshclient.EventPublishCommand, otherwise continue to check unixutil.IsPidRunning
as before; ensure the deferred panichandler.PanicHandler remains and optionally
log cancellation for clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f4b62645-a306-4eba-ad29-c8dc84c91833
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
frontend/preview/previews/tab.preview.tsxpackage.jsonpkg/util/unixutil/unixutil_unix.gopkg/wshrpc/wshremote/wshremote.go
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
Comment on lines
+47
to
+57
| { | ||
| tabId: "preview-tab-5", | ||
| tabName: "3 Badges", | ||
| active: false, | ||
| badges: [ | ||
| { badgeid: "b1", icon: "circle-small", color: "#fbbf24", priority: 1 }, | ||
| { badgeid: "b2", icon: "circle-check", color: "#4ade80", priority: 3 }, | ||
| { badgeid: "b3", icon: "triangle-exclamation", color: "#f87171", priority: 2 }, | ||
| { badgeid: "b4", icon: "bell", color: "#f87171", priority: 2 }, | ||
| ], | ||
| }, |
Contributor
There was a problem hiding this comment.
Tab name doesn't match badge count.
The tab is named "3 Badges" but contains 4 badge entries (b1, b2, b3, b4). Consider updating the name to "4 Badges" for consistency.
📝 Proposed fix
{
tabId: "preview-tab-5",
- tabName: "3 Badges",
+ tabName: "4 Badges",
active: false,
badges: [📝 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.
Suggested change
| { | |
| tabId: "preview-tab-5", | |
| tabName: "3 Badges", | |
| active: false, | |
| badges: [ | |
| { badgeid: "b1", icon: "circle-small", color: "#fbbf24", priority: 1 }, | |
| { badgeid: "b2", icon: "circle-check", color: "#4ade80", priority: 3 }, | |
| { badgeid: "b3", icon: "triangle-exclamation", color: "#f87171", priority: 2 }, | |
| { badgeid: "b4", icon: "bell", color: "#f87171", priority: 2 }, | |
| ], | |
| }, | |
| { | |
| tabId: "preview-tab-5", | |
| tabName: "4 Badges", | |
| active: false, | |
| badges: [ | |
| { badgeid: "b1", icon: "circle-small", color: "#fbbf24", priority: 1 }, | |
| { badgeid: "b2", icon: "circle-check", color: "#4ade80", priority: 3 }, | |
| { badgeid: "b3", icon: "triangle-exclamation", color: "#f87171", priority: 2 }, | |
| { badgeid: "b4", icon: "bell", color: "#f87171", priority: 2 }, | |
| ], | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/preview/previews/tab.preview.tsx` around lines 47 - 57, The tab
object with tabId "preview-tab-5" has tabName "3 Badges" but the badges array
contains four entries (badgeid "b1","b2","b3","b4"); update tabName to "4
Badges" (or programmatically derive the name from badges.length) so tabName
accurately reflects the number of badges in the badges array.
Comment on lines
+151
to
+173
| go func() { | ||
| defer func() { | ||
| panichandler.PanicHandler("BadgeWatchPidCommand", recover()) | ||
| }() | ||
| for { | ||
| time.Sleep(time.Second) | ||
| if unixutil.IsPidRunning(data.Pid) { | ||
| continue | ||
| } | ||
| orefStr := data.ORef.String() | ||
| event := wps.WaveEvent{ | ||
| Event: wps.Event_Badge, | ||
| Scopes: []string{orefStr}, | ||
| Data: baseds.BadgeEvent{ | ||
| ORef: orefStr, | ||
| ClearById: data.BadgeId, | ||
| }, | ||
| } | ||
| wshclient.EventPublishCommand(impl.RpcClient, event, nil) | ||
| log.Printf("BadgeWatchPidCommand: pid %d gone, cleared badge %s for oref %s\n", data.Pid, data.BadgeId, orefStr) | ||
| return | ||
| } | ||
| }() |
Contributor
There was a problem hiding this comment.
Stop the watcher when ctx is canceled.
This goroutine never observes ctx.Done(), so canceled or abandoned watches stay alive until the PID disappears and can still publish a late badge-clear event after the caller has gone away.
Suggested fix
go func() {
defer func() {
panichandler.PanicHandler("BadgeWatchPidCommand", recover())
}()
+ ticker := time.NewTicker(time.Second)
+ defer ticker.Stop()
for {
- time.Sleep(time.Second)
+ select {
+ case <-ctx.Done():
+ return
+ case <-ticker.C:
+ }
if unixutil.IsPidRunning(data.Pid) {
continue
}
+ select {
+ case <-ctx.Done():
+ return
+ default:
+ }
orefStr := data.ORef.String()
event := wps.WaveEvent{
Event: wps.Event_Badge,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/wshrpc/wshremote/wshremote.go` around lines 151 - 173, The goroutine
started in BadgeWatchPidCommand should stop when the provided ctx is canceled:
change the for loop to select on ctx.Done() and a timer/ticker (e.g., time.After
or time.Ticker) instead of unconditionally sleeping; when ctx.Done() fires
return immediately and do not call wshclient.EventPublishCommand, otherwise
continue to check unixutil.IsPidRunning as before; ensure the deferred
panichandler.PanicHandler remains and optionally log cancellation for clarity.
Comment on lines
+157
to
+159
| if unixutil.IsPidRunning(data.Pid) { | ||
| continue | ||
| } |
Contributor
There was a problem hiding this comment.
Verify the non-Unix IsPidRunning contract before depending on it here.
This watcher assumes unixutil.IsPidRunning is meaningful on every platform. If pkg/util/unixutil/unixutil_windows.go still returns false unconditionally, Windows will clear every badge on the first poll even while the process is still alive.
Run the following script to confirm the platform-specific implementations and the shared call site:
#!/bin/bash
set -euo pipefail
fd '^unixutil_.*\.go$' pkg/util/unixutil --exec sh -c '
for f do
echo "== $f =="
sed -n "1,200p" "$f"
done
' sh {} +
echo "== Badge watch call site =="
rg -n -C3 'BadgeWatchPidCommand|IsPidRunning' pkg/wshrpc/wshremote/wshremote.goExpected result: every OS-specific IsPidRunning implementation should honor the same contract: return true while the watched process still exists. If Windows still hardcodes false, this path needs a Windows implementation or an OS guard before enabling the command.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/wshrpc/wshremote/wshremote.go` around lines 157 - 159, The watcher in
wshremote.go calls unixutil.IsPidRunning(data.Pid) and assumes it returns true
while the process exists; verify all OS-specific implementations under
pkg/util/unixutil (especially unixutil_windows.go) adhere to that contract and,
if windows currently returns false unconditionally, either implement a correct
Windows IsPidRunning that checks process existence or add an OS guard in the
BadgeWatchPidCommand registration/path to avoid enabling the watcher on Windows;
locate and update the unixutil.IsPidRunning implementations or the
BadgeWatchPidCommand call site in wshremote.go accordingly so behavior is
consistent across platforms.
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
1 participant
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.
No description provided.