Repository navigation
feat(wework): add environment info popover - #1199
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds git-based device commands, threads optional project context through task open flows, implements environment helpers (parse/PR URL/load/commit) with tests, introduces an EnvironmentInfoPopover UI, and wires environment load/commit through workbench state and layouts. ChangesEnvironment Info and Project-Scoped Task Management
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
# Conflicts: # wework/src/api/devices.ts # wework/src/components/layout/DesktopSidebar.tsx # wework/src/components/layout/DesktopWorkbenchLayout.test.tsx # wework/src/features/workbench/workbenchReducer.test.ts # wework/src/features/workbench/workbenchReducer.ts # wework/src/pages/PluginManagementPage.tsx # wework/src/pages/PluginsPage.tsx
![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: 1
🧹 Nitpick comments (4)
wework/src/api/devices.ts (1)
30-46: 💤 Low valueConsider error handling consistency with
listDirectories.The
executeCommandmethod returns the rawDeviceCommandResponsewithout validation, whilelistDirectories(lines 15-29) validatesresponse.successand throws on errors. This difference may be intentional for flexibility sinceexecuteCommandis generic, but it places the validation burden on all callers.Consider documenting this design choice or adding an optional
validateSuccessparameter if consistent error handling would benefit common use cases.🤖 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 `@wework/src/api/devices.ts` around lines 30 - 46, executeCommand currently returns the raw DeviceCommandResponse unlike listDirectories which validates response.success and throws; either make behavior consistent by adding an optional boolean parameter validateSuccess (default false for backward compatibility) to executeCommand and when true check response.success and throw a descriptive error (matching listDirectories style), or add an inline doc comment above executeCommand explaining callers must validate response.success; update references to executeCommand and listDirectories accordingly so callers know the contract.backend/app/services/device/command_registry.py (2)
58-59: Write operations are intentional but high-privilege.The
git_add_allandgit_commitcommands modify repository state. While this is intentional for the environment commit feature and protected by device ownership validation, ensure that calling code handles these operations with appropriate user confirmation flows.🤖 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 `@backend/app/services/device/command_registry.py` around lines 58 - 59, The commands "git_add_all" and "git_commit" in the command registry (LocalDeviceCommandDefinition instances for git_add_all and git_commit) perform high-privilege write operations; ensure calling code that invokes these commands implements explicit user confirmation and respects existing device ownership validation before execution—update the callers of these command keys to prompt for confirmation, verify ownership via the device ownership check workflow, and log/record the confirmation result so these write operations are auditable.
24-38: ⚡ Quick winAdd documentation for the complex merge-base fallback logic.
The bash script implements a sophisticated fallback chain to find the appropriate merge-base for diff calculation, but the logic is not explained. Consider adding a docstring or inline comment documenting the fallback order and the rationale.
📝 Suggested documentation
+# GIT_BRANCH_DIFF_SHORTSTAT_COMMAND computes diff --shortstat from the merge-base +# of the current branch against the remote tracking branch. +# Fallback order: origin/HEAD → origin/main → main → origin/master → master → HEAD +# If no valid merge-base is found, falls back to diffing against HEAD. GIT_BRANCH_DIFF_SHORTSTAT_COMMAND = ( 'bash -lc \'base=""; '🤖 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 `@backend/app/services/device/command_registry.py` around lines 24 - 38, Add a short docstring or inline comment above the GIT_BRANCH_DIFF_SHORTSTAT_COMMAND constant in command_registry.py that explains the fallback order and rationale: describe that it first tries to read the remote origin HEAD symbolic ref, then attempts candidates in order (origin/main, main, origin/master, master), verifies candidate exists, falls back to showing full diff if none valid, computes merge-base between chosen base and HEAD, and if merge-base resolution fails also falls back to git diff --shortstat HEAD --; briefly note why these fallbacks are used (robustness across repos and shallow/absent remote refs) so future maintainers understand the logic.wework/scripts/dev-mac-app.sh (1)
1-3: ⚡ Quick winAdd an explicit macOS guard early.
Fail fast with a clear message when invoked on non-Darwin hosts; this avoids confusing command failures later in the script.
Suggested patch
#!/usr/bin/env bash set -euo pipefail + +if [ "$(uname -s)" != "Darwin" ]; then + echo "This script is macOS-only. Use pnpm dev on non-macOS environments." >&2 + exit 1 +fi🤖 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 `@wework/scripts/dev-mac-app.sh` around lines 1 - 3, Add an early macOS guard immediately after the shebang and set -euo pipefail lines: check uname -s (or OSTYPE) and if it is not Darwin, print a clear error message like "This script must be run on macOS (Darwin)." to stderr and exit non‑zero; place this guard at the top of dev-mac-app.sh so the script fails fast on non-Darwin hosts.
🤖 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 `@wework/src/components/layout/EnvironmentInfoPopover.tsx`:
- Around line 84-92: handleCopyDeviceId currently calls
navigator.clipboard?.writeText without guarding availability or handling
rejection, causing false success states; change it to check that
navigator.clipboard and navigator.clipboard.writeText exist, then await the
writeText call inside a try/catch, only call setDeviceCopied(true) and start the
timeout on successful completion, and in the catch log the error and/or surface
a failure state; reference the handleCopyDeviceId function, setDeviceCopied
state setter, and navigator.clipboard.writeText when making the changes.
---
Nitpick comments:
In `@backend/app/services/device/command_registry.py`:
- Around line 58-59: The commands "git_add_all" and "git_commit" in the command
registry (LocalDeviceCommandDefinition instances for git_add_all and git_commit)
perform high-privilege write operations; ensure calling code that invokes these
commands implements explicit user confirmation and respects existing device
ownership validation before execution—update the callers of these command keys
to prompt for confirmation, verify ownership via the device ownership check
workflow, and log/record the confirmation result so these write operations are
auditable.
- Around line 24-38: Add a short docstring or inline comment above the
GIT_BRANCH_DIFF_SHORTSTAT_COMMAND constant in command_registry.py that explains
the fallback order and rationale: describe that it first tries to read the
remote origin HEAD symbolic ref, then attempts candidates in order (origin/main,
main, origin/master, master), verifies candidate exists, falls back to showing
full diff if none valid, computes merge-base between chosen base and HEAD, and
if merge-base resolution fails also falls back to git diff --shortstat HEAD --;
briefly note why these fallbacks are used (robustness across repos and
shallow/absent remote refs) so future maintainers understand the logic.
In `@wework/scripts/dev-mac-app.sh`:
- Around line 1-3: Add an early macOS guard immediately after the shebang and
set -euo pipefail lines: check uname -s (or OSTYPE) and if it is not Darwin,
print a clear error message like "This script must be run on macOS (Darwin)." to
stderr and exit non‑zero; place this guard at the top of dev-mac-app.sh so the
script fails fast on non-Darwin hosts.
In `@wework/src/api/devices.ts`:
- Around line 30-46: executeCommand currently returns the raw
DeviceCommandResponse unlike listDirectories which validates response.success
and throws; either make behavior consistent by adding an optional boolean
parameter validateSuccess (default false for backward compatibility) to
executeCommand and when true check response.success and throw a descriptive
error (matching listDirectories style), or add an inline doc comment above
executeCommand explaining callers must validate response.success; update
references to executeCommand and listDirectories accordingly so callers know the
contract.
🪄 Autofix (Beta)
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
Run ID: 180b6937-de9a-47f8-86eb-190ac582d787
📒 Files selected for processing (31)
backend/app/schemas/task.pybackend/app/services/device/command_registry.pybackend/tests/services/test_local_device_command_service.pydocs/en/developer-guide/local-device-command-rpc.mddocs/zh/developer-guide/local-device-command-rpc.mdwework/package.jsonwework/scripts/dev-mac-app.shwework/src/api/devices.tswework/src/api/environment.test.tswework/src/api/environment.tswework/src/components/chat/ChatInput.test.tsxwework/src/components/chat/ChatInput.tsxwework/src/components/chat/composer/ProjectChatComposer.tsxwework/src/components/layout/DesktopSidebar.tsxwework/src/components/layout/DesktopWorkbenchLayout.test.tsxwework/src/components/layout/DesktopWorkbenchLayout.tsxwework/src/components/layout/DesktopWorkbenchMain.tsxwework/src/components/layout/EnvironmentInfoPopover.tsxwework/src/components/layout/MobileDrawer.tsxwework/src/components/layout/MobileWorkbenchLayout.tsxwework/src/components/layout/workspace-panels/WorkspacePanelActions.tsxwework/src/features/workbench/WorkbenchProvider.tsxwework/src/features/workbench/workbenchReducer.test.tswework/src/features/workbench/workbenchReducer.tswework/src/i18n/locales/en/common.jsonwework/src/i18n/locales/zh-CN/common.jsonwework/src/pages/PluginManagementPage.tsxwework/src/pages/PluginsPage.tsxwework/src/pages/WorkbenchPage.tsxwework/src/types/api.tswework/src/types/environment.ts
Comment on lines
+84
to
+92
| async function handleCopyDeviceId() { | ||
| if (!info.deviceId) { | ||
| return | ||
| } | ||
|
|
||
| await navigator.clipboard?.writeText(info.deviceId) | ||
| setDeviceCopied(true) | ||
| window.setTimeout(() => setDeviceCopied(false), 1200) | ||
| } |
Contributor
There was a problem hiding this comment.
Guard clipboard availability and handle write failures.
navigator.clipboard?.writeText(...) short-circuits to undefined in insecure contexts, so setDeviceCopied(true) still fires and falsely reports a successful copy. A rejected writeText (e.g. permissions) also escapes as an unhandled promise rejection since the onClick handler doesn't await it.
🛡️ Proposed fix
async function handleCopyDeviceId() {
- if (!info.deviceId) {
+ if (!info.deviceId || !navigator.clipboard) {
return
}
- await navigator.clipboard?.writeText(info.deviceId)
- setDeviceCopied(true)
- window.setTimeout(() => setDeviceCopied(false), 1200)
+ try {
+ await navigator.clipboard.writeText(info.deviceId)
+ setDeviceCopied(true)
+ window.setTimeout(() => setDeviceCopied(false), 1200)
+ } catch {
+ // clipboard write can reject (permissions); ignore
+ }
}🤖 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 `@wework/src/components/layout/EnvironmentInfoPopover.tsx` around lines 84 -
92, handleCopyDeviceId currently calls navigator.clipboard?.writeText without
guarding availability or handling rejection, causing false success states;
change it to check that navigator.clipboard and navigator.clipboard.writeText
exist, then await the writeText call inside a try/catch, only call
setDeviceCopied(true) and start the timeout on successful completion, and in the
catch log the error and/or surface a failure state; reference the
handleCopyDeviceId function, setDeviceCopied state setter, and
navigator.clipboard.writeText when making the changes.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wework/src/components/layout/DesktopWorkbenchLayout.tsx (1)
101-134:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRefresh environment info when the active project changes.
environmentInfoonly reloads after a completed assistant message or a manual refresh. On first render and after switching tasks/projects, this can leave the popover showing the previous project's branch/diff until some unrelated event triggersrefreshEnvironmentInfo().💡 Suggested fix
const refreshEnvironmentInfo = useCallback(async () => { setEnvironmentInfo(info => ({ ...info, loading: true })) try { const info = await onLoadEnvironmentInfo(environmentProject) setEnvironmentInfo({ ...info, loading: false }) } catch (error) { setEnvironmentInfo(info => ({ ...info, loading: false, error: error instanceof Error ? error.message : 'Failed to load environment info', })) } }, [environmentProject, onLoadEnvironmentInfo]) + + useEffect(() => { + void refreshEnvironmentInfo() + }, [refreshEnvironmentInfo]) useEffect(() => { const nextCompletedIds = new Set( messages .filter(message => message.role === 'assistant' && message.status === 'done')🤖 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 `@wework/src/components/layout/DesktopWorkbenchLayout.tsx` around lines 101 - 134, The environmentInfo isn't refreshed on mount or when environmentProject changes; add a new useEffect that calls refreshEnvironmentInfo whenever environmentProject (and refreshEnvironmentInfo) changes so the popover shows current project info immediately; reference the existing refreshEnvironmentInfo useCallback and the environmentProject prop/variable and ensure the effect runs on initial render by including those dependencies.
🤖 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.
Outside diff comments:
In `@wework/src/components/layout/DesktopWorkbenchLayout.tsx`:
- Around line 101-134: The environmentInfo isn't refreshed on mount or when
environmentProject changes; add a new useEffect that calls
refreshEnvironmentInfo whenever environmentProject (and refreshEnvironmentInfo)
changes so the popover shows current project info immediately; reference the
existing refreshEnvironmentInfo useCallback and the environmentProject
prop/variable and ensure the effect runs on initial render by including those
dependencies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 13340454-7509-4b2e-af38-3de96b36fa1b
📒 Files selected for processing (16)
backend/app/services/device/command_registry.pybackend/tests/services/test_local_device_command_service.pywework/src/api/devices.tswework/src/components/layout/DesktopSidebar.tsxwework/src/components/layout/DesktopWorkbenchLayout.test.tsxwework/src/components/layout/DesktopWorkbenchLayout.tsxwework/src/components/layout/MobileWorkbenchLayout.tsxwework/src/features/workbench/WorkbenchProvider.tsxwework/src/features/workbench/workbenchReducer.test.tswework/src/features/workbench/workbenchReducer.tswework/src/i18n/locales/en/common.jsonwework/src/i18n/locales/zh-CN/common.jsonwework/src/pages/PluginManagementPage.tsxwework/src/pages/PluginsPage.tsxwework/src/pages/WorkbenchPage.tsxwework/src/types/api.ts
✅ Files skipped from review due to trivial changes (2)
- wework/src/i18n/locales/en/common.json
- wework/src/i18n/locales/zh-CN/common.json
🚧 Files skipped from review as they are similar to previous changes (11)
- wework/src/types/api.ts
- wework/src/features/workbench/workbenchReducer.ts
- wework/src/components/layout/MobileWorkbenchLayout.tsx
- wework/src/pages/WorkbenchPage.tsx
- wework/src/pages/PluginManagementPage.tsx
- wework/src/features/workbench/workbenchReducer.test.ts
- backend/tests/services/test_local_device_command_service.py
- wework/src/components/layout/DesktopSidebar.tsx
- backend/app/services/device/command_registry.py
- wework/src/components/layout/DesktopWorkbenchLayout.test.tsx
- wework/src/features/workbench/WorkbenchProvider.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
start.sh (1)
1449-1452: ⚡ Quick winPrefer
while readfor line-by-line processing.The
for ... in $(...)pattern is less robust thanwhile IFS= read -rfor processing command output. While it works fine with the current simple service names, thewhile readapproach is more defensive and idiomatic in bash:
- Avoids word-splitting issues if output ever contains spaces
- Prevents glob expansion (
*,?)- More explicit about line-by-line iteration intent
♻️ Proposed fix to use the more robust pattern
local tracked_services=() local tracked_service - for tracked_service in $(get_tracked_services); do + while IFS= read -r tracked_service; do [ -n "$tracked_service" ] || continue tracked_services+=("$tracked_service") - done + done < <(get_tracked_services)🤖 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 `@start.sh` around lines 1449 - 1452, Replace the for-loop over command substitution with a safe line-oriented reader: run get_tracked_services through a process-substitution and use while IFS= read -r tracked_service; do [ -n "$tracked_service" ] || continue; tracked_services+=( "$tracked_service" ); done < <(get_tracked_services) so you avoid word-splitting, glob expansion, and ensure true line-by-line iteration when populating the tracked_services array.
🤖 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.
Nitpick comments:
In `@start.sh`:
- Around line 1449-1452: Replace the for-loop over command substitution with a
safe line-oriented reader: run get_tracked_services through a
process-substitution and use while IFS= read -r tracked_service; do [ -n
"$tracked_service" ] || continue; tracked_services+=( "$tracked_service" ); done
< <(get_tracked_services) so you avoid word-splitting, glob expansion, and
ensure true line-by-line iteration when populating the tracked_services array.
qdaxb
approved these changes
May 29, 2026
This was referenced May 29, 2026
This was referenced Jun 5, 2026
This was referenced Jun 16, 2026
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
2 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 by CodeRabbit
New Features
UI Improvements
Documentation
Tests