Repository navigation
feat(device): add remote Docker devices - #1455
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughIntroduces a new ChangesRemote Device (Docker) Support
Sequence Diagram(s)sequenceDiagram
participant Browser
participant AddCloudDeviceDialog
participant DeviceAPI
participant BackendEndpoint as POST /remote-devices/docker/start-command
participant APIKeyService
participant KindDB
Browser->>AddCloudDeviceDialog: click "生成 Docker 命令"
AddCloudDeviceDialog->>DeviceAPI: createDockerRemoteDeviceCommand({ client_origin })
DeviceAPI->>BackendEndpoint: POST /remote-devices/docker/start-command
BackendEndpoint->>BackendEndpoint: resolve backend URL + public base URL
BackendEndpoint->>APIKeyService: create_api_key_for_remote_device(db, user_id, user_name)
APIKeyService-->>BackendEndpoint: APIKey + plaintext token
BackendEndpoint->>KindDB: persist Device Kind CRD (deviceType=remote)
BackendEndpoint-->>DeviceAPI: DockerRemoteDeviceCommandResponse { command, env, device_id }
DeviceAPI-->>AddCloudDeviceDialog: response
AddCloudDeviceDialog->>Browser: render docker run command with copy button
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
![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: 7
🧹 Nitpick comments (5)
backend/tests/services/device/test_remote_provider.py (1)
18-18: ⚡ Quick winAnnotate the async test signature with explicit types
The new test function should include explicit parameter and return type annotations to satisfy the Python typing rule.
As per coding guidelines, "
**/*.py... type hints are required for Python code."🤖 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/tests/services/device/test_remote_provider.py` at line 18, The test function test_device_service_lists_remote_devices is missing type annotations for its parameters and return type. Add explicit type annotations to both the test_db and test_user parameters based on their expected types, and include a return type annotation of None for the async test function to comply with the Python typing requirements.Source: Coding guidelines
backend/tests/api/endpoints/test_remote_devices_api.py (1)
17-18: ⚡ Quick winAdd explicit type hints for test callables and helper init
This file is missing required type hints (e.g.,
__init__return type and async test signatures returningNone).As per coding guidelines, "
**/*.py... type hints are required for Python code."Also applies to: 27-31, 89-93
🤖 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/tests/api/endpoints/test_remote_devices_api.py` around lines 17 - 18, Add explicit return type hints to all method definitions in the test file to comply with coding guidelines. The `__init__` method in the `_FakeRequest` class needs a `-> None` return type hint. Additionally, all async test methods and helper functions referenced at lines 27-31 and 89-93 need appropriate return type hints (async test functions should return `None`). Review each method signature and add the missing return type annotations.Source: Coding guidelines
backend/app/api/endpoints/remote_devices.py (1)
275-284: ⚡ Quick winAdd OpenTelemetry tracing decorator.
Per coding guidelines, async functions should use
@trace_async()fromshared/telemetry/decorators.py.♻️ Proposed fix to add tracing
+from shared.telemetry.decorators import trace_async + `@router.post`( "/docker/start-command", response_model=DockerRemoteDeviceCommandResponse, ) +@trace_async() async def create_docker_start_command(🤖 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/api/endpoints/remote_devices.py` around lines 275 - 284, The async function create_docker_start_command is missing the required OpenTelemetry tracing decorator. Import the trace_async decorator from shared/telemetry/decorators.py and add the `@trace_async`() decorator directly above the function definition, before the `@router.post` decorator, to enable distributed tracing for this endpoint.Source: Coding guidelines
backend/app/services/api_key_service.py (1)
33-36: ⚡ Quick winConsider deferring commit to the caller for atomicity.
The function commits the API key immediately, but the caller (
create_docker_start_command) creates the Device CRD in a separate commit. If Device CRD creation fails, an orphaned API key remains in the database.♻️ Proposed fix to let caller manage transaction
- db.add(api_key) - db.commit() - db.refresh(api_key) + db.add(api_key) + db.flush() return api_key, full_keyThen ensure the caller commits both the API key and Device CRD together, or use a single transaction scope.
🤖 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/api_key_service.py` around lines 33 - 36, Remove the db.commit() call from the function containing the API key creation (the block with db.add(api_key), db.commit(), db.refresh(api_key)). Keep the db.add() and db.refresh() calls but defer the commit to the caller (create_docker_start_command). This allows the caller to manage a single transaction scope that includes both the API key creation and the Device CRD creation, ensuring atomicity so that if Device CRD creation fails, the API key is not persisted to the database.backend/app/services/device/remote_provider.py (1)
44-50: Replace== Truechecks with direct boolean filters forKind.is_active.The code uses
Kind.is_active == Truein database query filters, which violates PEP 8 (Ruff E712) and is non-idiomatic Python. Replace withKind.is_activedirectly.Proposed fix
- Kind.is_active == True, + Kind.is_active,Occurs at lines 49 and 111.
🤖 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/remote_provider.py` around lines 44 - 50, The query filter uses an explicit equality check Kind.is_active == True in the and_() condition, which violates PEP 8 style guidelines. Replace Kind.is_active == True with just Kind.is_active directly in both occurrences (at the current location and at line 111 in the same file) to make the boolean filter idiomatic and compliant with style standards.Sources: Coding guidelines, Linters/SAST tools
🤖 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 @.github/workflows/publish-image.yml:
- Around line 302-334: The GitHub Actions in the build-device-amd64,
build-device-arm64, and push-manifest jobs use unpinned version tags (like `@v4`,
`@v3`, `@v5`) instead of commit hashes, creating supply chain risk. Replace all
unpinned action references with their commit hash equivalents:
actions/checkout@v4 should become actions/checkout@<commit-hash>,
docker/setup-buildx-action@v3 should become
docker/setup-buildx-action@<commit-hash>, docker/login-action@v3 should become
docker/login-action@<commit-hash>, and docker/build-push-action@v5 should become
docker/build-push-action@<commit-hash>. Look up the current commit hash for each
action version from the GitHub Actions marketplace and update all occurrences
across the new device build jobs.
In `@backend/app/api/endpoints/remote_devices.py`:
- Around line 242-244: The transaction atomicity issue occurs because
`create_api_key_for_remote_device` performs a separate `db.commit()` for the API
key, and then `_create_remote_device_crd` performs another `db.commit()` for the
Device CRD. If the second operation fails, the first commit leaves an orphaned
API key in the database. To fix this, modify `create_api_key_for_remote_device`
to use `db.flush()` instead of `db.commit()` to stage the changes without
committing, and then move the single `db.commit()` call to the end of the
endpoint after both `create_api_key_for_remote_device` and
`_create_remote_device_crd` have completed successfully, ensuring both
operations are atomic.
In `@backend/app/services/device/remote_provider.py`:
- Around line 34-39: The async methods `get_status` and `list_devices` in the
remote provider are missing the `@trace_async()` decorator required by coding
guidelines for async public entry points. Add the `@trace_async()` decorator
above each of these method definitions to instrument them with OpenTelemetry
tracing, ensuring they are properly traced like other async public APIs in the
codebase.
In `@backend/tests/api/endpoints/test_remote_devices_api.py`:
- Line 55: The code contains SQLAlchemy filter conditions using `Kind.is_active
== True` which violates PEP 8 rule E712 and reduces query readability. Remove
the `== True` comparison and use the boolean column directly as a predicate in
both filter conditions (line 55 and line 80). Replace `Kind.is_active == True`
with just `Kind.is_active` since SQLAlchemy will correctly interpret the boolean
column without the explicit equality comparison.
In `@wework/src/components/settings/AddCloudDeviceDialog.tsx`:
- Around line 29-33: The handleCreate function contains hardcoded user-facing
strings that need to be converted to translation keys. Replace the hardcoded
error message '每个用户只能创建一个云设备。' in the setError call with a translation key.
Import the useTranslation hook from '`@/hooks/useTranslation`' (not from
'react-i18next') and use it to get the t function, then replace all hardcoded
strings mentioned in the review (lines 31, 45-46, 66-67, 97-100, 127-132,
154-157, 166, 173, 181, 204) with t() calls using appropriate translation keys.
Additionally, add the new translation keys to both the Chinese (zh) and English
(en) locale namespace files without any fallback literals, ensuring complete
translations are provided in both languages.
- Around line 83-85: The backdrop click handler and other close paths (X button
close and cancel button) in the AddCloudDeviceDialog component are
inconsistently checking the loading state. Update all close paths to check both
!loading and !remoteLoading conditions to ensure no closes are allowed while a
remote request is in progress. Specifically, in the backdrop onClick handler
around line 83-85, the cancel button handler around line 103-107, and the X
button close around line 201, add !remoteLoading to each condition so that all
close mechanisms are consistently disabled during remote operations.
In `@wework/src/components/settings/ConnectionsSettingsPage.tsx`:
- Around line 326-340: The code contains hardcoded Chinese literal strings for
the delete dialog title and description variables instead of using translation
keys. Replace all the hardcoded strings in the title and description variables
(which include '删除云设备', '删除远程设备', '删除本地设备', '重启云设备', and their corresponding
descriptions) with translation key references using the useTranslation hook
imported from `@/hooks/useTranslation`. Add corresponding translation key entries
to both the Chinese and English locale files with the appropriate translations
for each scenario (cloud device, remote device, and local device delete
operations).
---
Nitpick comments:
In `@backend/app/api/endpoints/remote_devices.py`:
- Around line 275-284: The async function create_docker_start_command is missing
the required OpenTelemetry tracing decorator. Import the trace_async decorator
from shared/telemetry/decorators.py and add the `@trace_async`() decorator
directly above the function definition, before the `@router.post` decorator, to
enable distributed tracing for this endpoint.
In `@backend/app/services/api_key_service.py`:
- Around line 33-36: Remove the db.commit() call from the function containing
the API key creation (the block with db.add(api_key), db.commit(),
db.refresh(api_key)). Keep the db.add() and db.refresh() calls but defer the
commit to the caller (create_docker_start_command). This allows the caller to
manage a single transaction scope that includes both the API key creation and
the Device CRD creation, ensuring atomicity so that if Device CRD creation
fails, the API key is not persisted to the database.
In `@backend/app/services/device/remote_provider.py`:
- Around line 44-50: The query filter uses an explicit equality check
Kind.is_active == True in the and_() condition, which violates PEP 8 style
guidelines. Replace Kind.is_active == True with just Kind.is_active directly in
both occurrences (at the current location and at line 111 in the same file) to
make the boolean filter idiomatic and compliant with style standards.
In `@backend/tests/api/endpoints/test_remote_devices_api.py`:
- Around line 17-18: Add explicit return type hints to all method definitions in
the test file to comply with coding guidelines. The `__init__` method in the
`_FakeRequest` class needs a `-> None` return type hint. Additionally, all async
test methods and helper functions referenced at lines 27-31 and 89-93 need
appropriate return type hints (async test functions should return `None`).
Review each method signature and add the missing return type annotations.
In `@backend/tests/services/device/test_remote_provider.py`:
- Line 18: The test function test_device_service_lists_remote_devices is missing
type annotations for its parameters and return type. Add explicit type
annotations to both the test_db and test_user parameters based on their expected
types, and include a return type annotation of None for the async test function
to comply with the Python typing requirements.
🪄 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: 0c855891-53c8-4caa-bf63-ff5f46bf045d
📒 Files selected for processing (26)
.github/workflows/publish-image.ymlbackend/app/api/api.pybackend/app/api/endpoints/admin/device_monitor.pybackend/app/api/endpoints/remote_devices.pybackend/app/api/ws/device_namespace.pybackend/app/schemas/device.pybackend/app/services/api_key_service.pybackend/app/services/device/local_provider.pybackend/app/services/device/provider_factory.pybackend/app/services/device/remote_provider.pybackend/app/services/device_service.pybackend/tests/api/endpoints/test_remote_devices_api.pybackend/tests/services/device/test_remote_provider.pydocs/en/developer-guide/local-device-architecture.mddocs/en/reference/yaml-specification.mddocs/en/user-guide/ai-devices/local-device-support.mddocs/zh/developer-guide/local-device-architecture.mddocs/zh/reference/yaml-specification.mddocs/zh/user-guide/ai-devices/local-device-support.mdexecutor/config/device_config.pywework/src/api/devices.tswework/src/components/settings/AddCloudDeviceDialog.tsxwework/src/components/settings/ConnectionsSettingsPage.test.tsxwework/src/components/settings/ConnectionsSettingsPage.tsxwework/src/lib/device-capabilities.tswework/src/types/devices.ts
Comment on lines
+302
to
+334
| build-device-amd64: | ||
| needs: prepare-release | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: read | ||
| packages: write | ||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| ref: ${{ env.GIT_REF }} | ||
|
|
||
| - name: Set up Docker Buildx | ||
| uses: docker/setup-buildx-action@v3 | ||
|
|
||
| - name: Log in to GitHub Container Registry | ||
| uses: docker/login-action@v3 | ||
| with: | ||
| registry: ${{ env.REGISTRY }} | ||
| username: ${{ github.actor }} | ||
| password: ${{ secrets.GITHUB_TOKEN }} | ||
|
|
||
| - name: Build and push device image (amd64) | ||
| uses: docker/build-push-action@v5 | ||
| with: | ||
| context: . | ||
| file: docker/device/Dockerfile | ||
| push: true | ||
| platforms: linux/amd64 | ||
| tags: ${{ env.IMAGE_PREFIX }}/wegent-device:${{ needs.prepare-release.outputs.new_version }}-amd64 | ||
| cache-from: type=gha,scope=device-amd64 | ||
| cache-to: type=gha,mode=max,scope=device-amd64 | ||
|
|
Contributor
There was a problem hiding this comment.
Unpinned GitHub Actions increase supply chain risk.
The new device image build jobs use unpinned action references:
actions/checkout@v4(lines 310, 343)docker/setup-buildx-action@v3(lines 315, 348)docker/login-action@v3(lines 318, 351)docker/build-push-action@v5(lines 325, 358)docker/login-action@v3in manifest job (line 383)docker/setup-buildx-action@v3in manifest job (line 390)
These should be pinned to specific commit hashes to prevent supply chain attacks. For example, replace actions/checkout@v4 with actions/checkout@<commit-hash>.
Note: This pattern is pre-existing in other jobs within the file (build-backend, build-executor, etc.), so you may want to address all unpinned actions in a separate pass or policy enforcement. However, new jobs should follow secure defaults.
Also applies to: 335-367, 372-407
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 309-312: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 310-310: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 315-315: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 318-318: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 325-325: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/publish-image.yml around lines 302 - 334, The GitHub
Actions in the build-device-amd64, build-device-arm64, and push-manifest jobs
use unpinned version tags (like `@v4`, `@v3`, `@v5`) instead of commit hashes,
creating supply chain risk. Replace all unpinned action references with their
commit hash equivalents: actions/checkout@v4 should become
actions/checkout@<commit-hash>, docker/setup-buildx-action@v3 should become
docker/setup-buildx-action@<commit-hash>, docker/login-action@v3 should become
docker/login-action@<commit-hash>, and docker/build-push-action@v5 should become
docker/build-push-action@<commit-hash>. Look up the current commit hash for each
action version from the GitHub Actions marketplace and update all occurrences
across the new device build jobs.
Source: Linters/SAST tools
Comment on lines
+242
to
+244
| db.add(device_kind) | ||
| db.commit() | ||
| db.refresh(device_kind) |
Contributor
There was a problem hiding this comment.
Transaction atomicity: API key and Device CRD commits are separate.
create_api_key_for_remote_device commits the API key, then _create_remote_device_crd commits the Device CRD separately. If the second commit fails, an orphaned API key remains. Consider wrapping both operations in a single transaction.
🛡️ Proposed fix for atomic transaction
Modify create_api_key_for_remote_device to use db.flush() instead of db.commit(), then commit once after both records are created:
# In api_key_service.py
- db.commit()
+ db.flush()
# In remote_devices.py, after _create_remote_device_crd:
+ db.commit()Or use a single commit at the end of the endpoint after both operations.
Also applies to: 298-313
🤖 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/api/endpoints/remote_devices.py` around lines 242 - 244, The
transaction atomicity issue occurs because `create_api_key_for_remote_device`
performs a separate `db.commit()` for the API key, and then
`_create_remote_device_crd` performs another `db.commit()` for the Device CRD.
If the second operation fails, the first commit leaves an orphaned API key in
the database. To fix this, modify `create_api_key_for_remote_device` to use
`db.flush()` instead of `db.commit()` to stage the changes without committing,
and then move the single `db.commit()` call to the end of the endpoint after
both `create_api_key_for_remote_device` and `_create_remote_device_crd` have
completed successfully, ensuring both operations are atomic.
Comment on lines
+34
to
+39
| async def get_status( | ||
| self, | ||
| db: Session, | ||
| user_id: int, | ||
| device_id: str, | ||
| ) -> Optional[Dict[str, Any]]: |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add tracing decorators to the new async provider APIs.
get_status and list_devices are new async public entry points, but they are not instrumented with @trace_async().
Proposed fix
from app.services.device.version_service import executor_version_service
+from shared.telemetry.decorators import trace_async
@@
+ `@trace_async`()
async def get_status(
@@
+ `@trace_async`()
async def list_devices(As per coding guidelines, **/*.py must use OpenTelemetry tracing decorators (@trace_async() for async functions).
Also applies to: 97-103
🤖 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/remote_provider.py` around lines 34 - 39, The
async methods `get_status` and `list_devices` in the remote provider are missing
the `@trace_async()` decorator required by coding guidelines for async public
entry points. Add the `@trace_async()` decorator above each of these method
definitions to instrument them with OpenTelemetry tracing, ensuring they are
properly traced like other async public APIs in the codebase.
Source: Coding guidelines
| Kind.kind == "Device", | ||
| Kind.namespace == "default", | ||
| Kind.name == response.device_id, | ||
| Kind.is_active == True, |
Contributor
There was a problem hiding this comment.
Use SQLAlchemy boolean predicates directly in filters
Line 55 and Line 80 use == True, which violates PEP 8 (E712) and reduces query readability.
Suggested fix
.filter(
Kind.user_id == test_user.id,
Kind.kind == "Device",
Kind.namespace == "default",
Kind.name == response.device_id,
- Kind.is_active == True,
+ Kind.is_active,
)
@@
.filter(
APIKey.user_id == test_user.id,
APIKey.name == f"{test_user.user_name}-remote-device",
- APIKey.is_active == True,
+ APIKey.is_active,
)As per coding guidelines, "**/*.py: Use PEP 8 ..."; static analysis also flags this as E712.
Also applies to: 80-80
🧰 Tools
🪛 Ruff (0.15.17)
[error] 55-55: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
🤖 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/tests/api/endpoints/test_remote_devices_api.py` at line 55, The code
contains SQLAlchemy filter conditions using `Kind.is_active == True` which
violates PEP 8 rule E712 and reduces query readability. Remove the `== True`
comparison and use the boolean column directly as a predicate in both filter
conditions (line 55 and line 80). Replace `Kind.is_active == True` with just
`Kind.is_active` since SQLAlchemy will correctly interpret the boolean column
without the explicit equality comparison.
Sources: Coding guidelines, Linters/SAST tools
Comment on lines
29
to
+33
| const handleCreate = useCallback(async () => { | ||
| if (hasCloudDevice) { | ||
| setError('每个用户只能创建一个云设备。') | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
Route newly added dialog copy through translation keys (no inline/fallback literals).
This new flow introduces many hardcoded/fallback user strings, which breaks the repo’s i18n rule for new UI code.
Proposed direction
+import { useTranslation } from '`@/hooks/useTranslation`'
...
export function AddCloudDeviceDialog({
...
}: AddCloudDeviceDialogProps) {
+ const { t } = useTranslation('common')
...
- if (hasCloudDevice) {
- setError('每个用户只能创建一个云设备。')
+ if (hasCloudDevice) {
+ setError(t('workbench.connection_cloud_device_single_limit'))
return
}
...
- setError(e instanceof Error ? e.message : '创建失败，请重试')
+ setError(e instanceof Error ? e.message : t('common.create_failed_retry'))
...
- setError(e instanceof Error ? e.message : '生成远程设备命令失败，请重试')
+ setError(e instanceof Error ? e.message : t('workbench.connection_remote_command_failed'))
...
- <h2 className="text-sm font-semibold text-text-primary">添加设备</h2>
+ <h2 className="text-sm font-semibold text-text-primary">
+ {t('workbench.connection_add_device_title')}
+ </h2>
...
- <h3 className="text-sm font-semibold text-text-primary">云设备</h3>
+ <h3 className="text-sm font-semibold text-text-primary">
+ {t('workbench.connection_cloud_devices')}
+ </h3>Also add the new keys in both locale namespaces (zh + en), and avoid fallback literals in the new entries.
As per coding guidelines, “Import from '@/hooks/useTranslation' for i18n in application code, not from 'react-i18next'” and “Do not add fallback literals to hide missing translations in new code. Add the key to the correct namespace in both languages instead.”
Also applies to: 45-46, 66-67, 97-100, 127-132, 154-157, 166-166, 173-173, 181-181, 204-204
🤖 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/settings/AddCloudDeviceDialog.tsx` around lines 29 -
33, The handleCreate function contains hardcoded user-facing strings that need
to be converted to translation keys. Replace the hardcoded error message
'每个用户只能创建一个云设备。' in the setError call with a translation key. Import the
useTranslation hook from '`@/hooks/useTranslation`' (not from 'react-i18next') and
use it to get the t function, then replace all hardcoded strings mentioned in
the review (lines 31, 45-46, 66-67, 97-100, 127-132, 154-157, 166, 173, 181,
204) with t() calls using appropriate translation keys. Additionally, add the
new translation keys to both the Chinese (zh) and English (en) locale namespace
files without any fallback literals, ensuring complete translations are provided
in both languages.
Source: Coding guidelines
Comment on lines
83
to
85
| onClick={e => { | ||
| if (!loading && e.target === e.currentTarget) onClose() | ||
| }} |
Contributor
There was a problem hiding this comment.
Keep all close paths disabled while remoteLoading is true.
Backdrop/X close currently ignore remoteLoading, but cancel already blocks it. This allows mid-request close with inconsistent UX/state behavior.
Suggested fix
onClick={e => {
- if (!loading && e.target === e.currentTarget) onClose()
+ if (!loading && !remoteLoading && e.target === e.currentTarget) onClose()
}}
...
- disabled={loading}
+ disabled={loading || remoteLoading}Also applies to: 103-107, 201-201
🤖 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/settings/AddCloudDeviceDialog.tsx` around lines 83 -
85, The backdrop click handler and other close paths (X button close and cancel
button) in the AddCloudDeviceDialog component are inconsistently checking the
loading state. Update all close paths to check both !loading and !remoteLoading
conditions to ensure no closes are allowed while a remote request is in
progress. Specifically, in the backdrop onClick handler around line 83-85, the
cancel button handler around line 103-107, and the X button close around line
201, add !remoteLoading to each condition so that all close mechanisms are
consistently disabled during remote operations.
Comment on lines
+326
to
+340
| const isRemote = isRemoteDevice(device) | ||
| const Icon = isDelete ? Trash2 : RotateCcw | ||
| const title = isDelete ? (isCloud ? '删除云设备' : '删除本地设备') : '重启云设备' | ||
| const title = isDelete | ||
| ? isCloud | ||
| ? '删除云设备' | ||
| : isRemote | ||
| ? '删除远程设备' | ||
| : '删除本地设备' | ||
| : '重启云设备' | ||
| const description = isDelete | ||
| ? isCloud | ||
| ? `将删除 ${device.name}，相关云设备资源会被释放。` | ||
| : `将删除 ${device.name} 的本地设备注册记录。设备重新连接后会自动重新注册。` | ||
| : isRemote | ||
| ? `将删除 ${device.name} 的远程设备注册记录。Docker 容器需要你自行停止或删除。` | ||
| : `将删除 ${device.name} 的本地设备注册记录。设备重新连接后会自动重新注册。` |
Contributor
There was a problem hiding this comment.
Use translation keys (without fallback literals) for new remote-device copy.
The new delete-dialog strings and remote section title introduce new literal/fallback text instead of key-only i18n usage.
Suggested change
function ConfirmDeviceActionDialog({
...
}) {
+ const { t } = useTranslation('common')
...
const title = isDelete
? isCloud
- ? '删除云设备'
+ ? t('workbench.connection_delete_cloud_device_title')
: isRemote
- ? '删除远程设备'
- : '删除本地设备'
- : '重启云设备'
+ ? t('workbench.connection_delete_remote_device_title')
+ : t('workbench.connection_delete_local_device_title')
+ : t('workbench.connection_restart_cloud_device_title')
...
const description = isDelete
? isCloud
- ? `将删除 ${device.name}，相关云设备资源会被释放。`
+ ? t('workbench.connection_delete_cloud_device_desc', { name: device.name })
: isRemote
- ? `将删除 ${device.name} 的远程设备注册记录。Docker 容器需要你自行停止或删除。`
- : `将删除 ${device.name} 的本地设备注册记录。设备重新连接后会自动重新注册。`
+ ? t('workbench.connection_delete_remote_device_desc', { name: device.name })
+ : t('workbench.connection_delete_local_device_desc', { name: device.name })
...
- title={t('workbench.connection_remote_devices', '远程设备')}
+ title={t('workbench.connection_remote_devices')}Add these new keys in both zh/en locale files.
As per coding guidelines, “Import from '@/hooks/useTranslation' for i18n in application code, not from 'react-i18next'” and “Do not add fallback literals to hide missing translations in new code. Add the key to the correct namespace in both languages instead.”
Also applies to: 1060-1060
🤖 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/settings/ConnectionsSettingsPage.tsx` around lines 326
- 340, The code contains hardcoded Chinese literal strings for the delete dialog
title and description variables instead of using translation keys. Replace all
the hardcoded strings in the title and description variables (which include
'删除云设备', '删除远程设备', '删除本地设备', '重启云设备', and their corresponding descriptions) with
translation key references using the useTranslation hook imported from
`@/hooks/useTranslation`. Add corresponding translation key entries to both the
Chinese and English locale files with the appropriate translations for each
scenario (cloud device, remote device, and local device delete operations).
Source: Coding guidelines
…-support # Conflicts: # docs/en/user-guide/ai-devices/local-device-support.md # docs/zh/user-guide/ai-devices/local-device-support.md # wework/src/components/settings/AddCloudDeviceDialog.tsx # wework/src/lib/device-capabilities.ts
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)
backend/app/api/endpoints/admin/device_monitor.py (1)
107-109: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate docstring to reflect REMOTE device type support.
The field docstring at line 108 describes only "(local, cloud)" but the code now tracks REMOTE devices as well.
📝 Proposed fix
- by_device_type: Dict[str, int] = Field( - ..., description="Count by device type (local, cloud)" - ) + by_device_type: Dict[str, int] = Field( + ..., description="Count by device type (local, cloud, remote)" + )🤖 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/api/endpoints/admin/device_monitor.py` around lines 107 - 109, The docstring for the by_device_type field currently describes only local and cloud device types, but the code now also tracks REMOTE devices. Update the description parameter in the Field definition for by_device_type to include all three supported device types: local, cloud, and remote. This will ensure the documentation accurately reflects what device types are being counted.
🧹 Nitpick comments (1)
backend/app/api/endpoints/admin/device_monitor.py (1)
713-734: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse explicit exception chaining when re-raising in exception handlers.
The code raises a new HTTPException without preserving the original exception context. Use
raise ... from eto maintain the exception chain for better debugging. Additionally, replacestr(e)with the explicit conversion flag{e!s}for consistency.🔗 Proposed fix for exception chaining and conversion
try: result = await restart_admin_device(db, user_id, device_id) except HTTPException: raise except Exception as e: logger.exception( f"[Admin Device Restart] Error: device_id={device_id}, error={e}" ) raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail=f"Failed to trigger restart: {str(e)}", - ) + detail=f"Failed to trigger restart: {e!s}", + ) from e🤖 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/api/endpoints/admin/device_monitor.py` around lines 713 - 734, The exception handler in the restart_admin_device endpoint catches a generic Exception but re-raises a new HTTPException without preserving the original exception context. Change the raise HTTPException statement to use explicit exception chaining with `raise ... from e` syntax to maintain the exception chain for better debugging. Additionally, in the detail parameter of HTTPException, replace `str(e)` with the f-string conversion flag `{e!s}` for consistency with modern Python practices.Source: Linters/SAST tools
🤖 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 `@backend/app/api/endpoints/admin/device_monitor.py`:
- Around line 107-109: The docstring for the by_device_type field currently
describes only local and cloud device types, but the code now also tracks REMOTE
devices. Update the description parameter in the Field definition for
by_device_type to include all three supported device types: local, cloud, and
remote. This will ensure the documentation accurately reflects what device types
are being counted.
---
Nitpick comments:
In `@backend/app/api/endpoints/admin/device_monitor.py`:
- Around line 713-734: The exception handler in the restart_admin_device
endpoint catches a generic Exception but re-raises a new HTTPException without
preserving the original exception context. Change the raise HTTPException
statement to use explicit exception chaining with `raise ... from e` syntax to
maintain the exception chain for better debugging. Additionally, in the detail
parameter of HTTPException, replace `str(e)` with the f-string conversion flag
`{e!s}` for consistency with modern Python practices.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6bcf4800-e6ad-4b3b-b9d2-2e6ffd521f11
📒 Files selected for processing (9)
backend/app/api/api.pybackend/app/api/endpoints/admin/device_monitor.pybackend/app/api/ws/device_namespace.pybackend/app/schemas/device.pybackend/app/services/device/local_provider.pybackend/app/services/device_service.pychat_shell/tests/test_dockerfile.pydocs/en/user-guide/ai-devices/local-device-support.mddocs/zh/user-guide/ai-devices/local-device-support.md
💤 Files with no reviewable changes (3)
- chat_shell/tests/test_dockerfile.py
- docs/en/user-guide/ai-devices/local-device-support.md
- docs/zh/user-guide/ai-devices/local-device-support.md
✅ Files skipped from review due to trivial changes (2)
- backend/app/services/device_service.py
- backend/app/services/device/local_provider.py
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/app/api/api.py
- backend/app/schemas/device.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@backend/tests/services/test_local_device_provider.py`:
- Line 38: Add explicit type hints to the function signature
`test_list_devices_excludes_remote_devices`. Add a type hint for the `test_db`
parameter (which appears to be a pytest fixture) and include the `-> None`
return type annotation on the function. This ensures compliance with the
repository guideline requiring type hints for all function parameters and return
values.
🪄 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: fe40c597-3ca1-4569-b28d-4cacd6a78f39
📒 Files selected for processing (1)
backend/tests/services/test_local_device_provider.py
| ): | ||
| """Legacy remote devices are local devices in the current API contract.""" | ||
| test_db.add(_local_device("legacy-remote", "remote")) | ||
| async def test_list_devices_excludes_remote_devices(test_db): |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add explicit type hints to the async test signature.
test_list_devices_excludes_remote_devices is missing type hints for test_db and return type (-> None), which violates the repository Python guideline.
As per coding guidelines, "**/*.py: Use type hints for all function parameters and return values".
🤖 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/tests/services/test_local_device_provider.py` at line 38, Add
explicit type hints to the function signature
`test_list_devices_excludes_remote_devices`. Add a type hint for the `test_db`
parameter (which appears to be a pytest fixture) and include the `-> None`
return type annotation on the function. This ensures compliance with the
repository guideline requiring type hints for all function parameters and return
values.
Source: Coding guidelines
This was referenced Jun 25, 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
runcommands.