Tuya - local baseline - PR1#2434
Conversation
…er models - Added support for air conditioning devices with new mappings and DPS configurations. - Introduced local polling for Tuya devices to improve responsiveness. - Enhanced device conversion logic to include additional parameters such as cloud IP and local override. - Updated feature conversion to utilize advanced DPS mappings for air conditioning. - Implemented new models for air conditioning and power meter, including specific feature mappings. - Improved error handling and logging for local polling and device value setting. - Added unit tests for new feature mappings and conversion logic.
…y device conversion logic
#4133 Bundle Size — 11.51MiB (+0.62%).19ff616(current) vs b3cfa5a master#4120(baseline) Warning Bundle contains 2 duplicate packages – View duplicate packages Bundle metrics
|
| Current #4133 |
Baseline #4120 |
|
|---|---|---|
 Initial JS |
6.49MiB(+1.11%) |
6.42MiB |
| Initial CSS |
310.6KiB(+0.02%) |
310.54KiB |
 Cache Invalidation |
58.65% |
0% |
 Chunks |
51 |
51 |
| Assets |
179 |
179 |
| Modules |
1643 |
1643 |
| Duplicate Modules |
21 |
21 |
| Duplicate Code |
0.94% |
0.94% |
| Packages |
136 |
136 |
| Duplicate Packages |
2 |
2 |
Bundle size by type 2 changes 2 regressions
| Current #4133 |
Baseline #4120 |
|
|---|---|---|
| JS |
8.39MiB (+0.86%) |
8.32MiB |
| IMG |
2.68MiB |
2.68MiB |
| CSS |
328.45KiB (+0.02%) |
328.39KiB |
| Fonts |
93.55KiB |
93.55KiB |
| Other |
18.82KiB |
18.82KiB |
| HTML |
13.58KiB |
13.58KiB |
Bundle analysis report Branch Terdious:tuya-local-baseline Project dashboard
Generated by RelativeCI Documentation Report issue
…ttre à jour les tests associés
… champ d'erreur dans le payload de l'événement WebSocket
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2434 +/- ##
========================================
Coverage 98.81% 98.81%
========================================
Files 1009 1017 +8
Lines 17670 18235 +565
========================================
+ Hits 17460 18019 +559
- Misses 210 216 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…s et ajouter des tests pour la gestion des appareils locaux
…on Tuya dans les fichiers de langue
…ion Tuya et mettre à jour les traductions
… des appareils Tuya
…tre l'utilisation d'adresses spécifiques
…eurs de port et mise à jour des traductions
…re des liens vers la documentation et les options de connexion
…réation de rapports GitHub pour les appareils Tuya
…l disconnect features - Added new translations for connection status messages in German, English, and French. - Implemented API endpoints to get Tuya connection status and to manually disconnect from the Tuya cloud. - Updated the Tuya service to handle automatic reconnection logic and manual disconnect state. - Enhanced the SetupTab component to reflect connection status and provide a disconnect button. - Added tests for the new functionality, including status retrieval and manual disconnect.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds local UDP discovery and TCP polling for Tuya devices, extensive frontend i18n/UI updates for local/cloud flows, new backend local-scan/local-poll/status/disconnect endpoints, device param normalization and local write paths, auto-reconnect/manual-disconnect handling, and many unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/UI
participant Frontend as Frontend
participant Backend as Backend
participant LocalDevice as Local Tuya Device
participant TuyaCloud as Tuya Cloud
User->>Frontend: Trigger "Local Scan"
Frontend->>Backend: POST /api/v1/service/tuya/local-scan
Backend->>LocalDevice: Bind UDP ports (6666/6667/7000) and listen
LocalDevice-->>Backend: UDP discovery packets
Backend->>Backend: Parse packets, aggregate local devices & portErrors
Backend-->>Frontend: Return local_devices + portErrors
Frontend->>User: Display local discovery results
User->>Frontend: Click "Poll Local" on device
Frontend->>Backend: POST /api/v1/service/tuya/local-poll (id, ip, key, proto)
Backend->>LocalDevice: TCP connect (try 3.5 → 3.4 → 3.3 → 3.1)
LocalDevice-->>Backend: DPS response
Backend->>Backend: Update discovered device params (ip, protocol, local_key)
Backend-->>Frontend: Return poll result (dps or error)
Frontend->>User: Show poll success/error
sequenceDiagram
participant SetupUI as Setup UI
participant API as Backend API
participant TuyaService as Tuya Service
participant Gladys as Gladys Variables
participant WebSocket as WebSocket
SetupUI->>API: GET configuration (endpoint, accessKey, secretKey, appUsername)
API->>Gladys: gladys.variable.getValue(...) xN
Gladys-->>API: config values
API-->>SetupUI: configuration loaded
SetupUI->>API: POST /api/v1/service/tuya/status
API->>TuyaService: tuyaManager.getStatus()
TuyaService->>Gladys: read MANUAL_DISCONNECT & credentials
Gladys-->>TuyaService: values
TuyaService-->>API: { status, connected, configured, error, manual_disconnect }
API-->>SetupUI: status result
WebSocket-->>SetupUI: TUYA.STATUS events update UI in real-time
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
front/src/routes/integration/all/tuya/setup-page/SetupTab.jsx (1)
144-179:⚠️ Potential issue | 🟡 MinorCompute “configured” using trimmed credentials.
configuredis derived from raw state, but you.trim()before saving. Whitespace-only values will mark the UI as configured and attempt a start, while saving empty strings. Trim once and reuse the trimmed values for both saving and config checks.🐛 Suggested fix
- await this.props.httpClient.post('/api/v1/service/tuya/variable/TUYA_ACCESS_KEY', { - value: this.state.tuyaAccessKey.trim() - }); + const tuyaEndpoint = this.state.tuyaEndpoint.trim(); + const tuyaAccessKey = this.state.tuyaAccessKey.trim(); + const tuyaSecretKey = this.state.tuyaSecretKey.trim(); + const tuyaAppAccountId = this.state.tuyaAppAccountId.trim(); + const tuyaAppUsername = this.state.tuyaAppUsername.trim(); + + await this.props.httpClient.post('/api/v1/service/tuya/variable/TUYA_ACCESS_KEY', { + value: tuyaAccessKey + }); ... - const configured = !!(this.state.tuyaEndpoint && this.state.tuyaAccessKey && this.state.tuyaSecretKey); + const configured = !!(tuyaEndpoint && tuyaAccessKey && tuyaSecretKey);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front/src/routes/integration/all/tuya/setup-page/SetupTab.jsx` around lines 144 - 179, The configured check uses raw state while you trim values before saving, so create trimmed local variables (e.g., const tuyaAccessKey = this.state.tuyaAccessKey.trim(), const tuyaSecretKey = this.state.tuyaSecretKey.trim(), and trim tuyaAppAccountId and tuyaAppUsername similarly), use those trimmed variables in the httpClient.post calls (instead of this.state.*.trim() inline) and compute configured with the trimmed credentials (e.g., const configured = !!(this.state.tuyaEndpoint && tuyaAccessKey && tuyaSecretKey)); update any subsequent logic that relies on the values to use the trimmed variables.
🧹 Nitpick comments (7)
front/src/config/i18n/en.json (3)
1121-1121: Consider clarifying scan progress message for both cloud and local scans.The message "Scan in progress... retrieving cloud devices." only mentions cloud devices, but based on the PR objectives, the scan functionality now includes both cloud and local device discovery. Consider updating to something like "Scan in progress..." or "Scan in progress... retrieving devices." to be accurate for both scan types.
Suggested fix
- "scanInProgress": "Scan in progress... retrieving cloud devices.", + "scanInProgress": "Scan in progress...",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front/src/config/i18n/en.json` at line 1121, The "scanInProgress" i18n message currently reads "Scan in progress... retrieving cloud devices." but the scan now covers both cloud and local devices; update the translation for the "scanInProgress" key to a neutral phrase such as "Scan in progress..." or "Scan in progress... retrieving devices." so it accurately reflects both cloud and local scans—locate the "scanInProgress" entry in en.json and replace the string accordingly.
1166-1168: Consider consolidating duplicate success messages.Lines 1166-1168 contain very similar messages:
connected: "Connected to the Tuya cloud account with success !"connectedAfterSave: "Connected to the Tuya cloud account with success !"connectedStatus: "Connected to the Tuya cloud account."The first two are identical. If they serve different UI purposes, consider differentiating them (e.g., one could include additional context). If not, consider using a single key to avoid maintenance overhead and ensure consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front/src/config/i18n/en.json` around lines 1166 - 1168, The three i18n keys connected, connectedAfterSave, and connectedStatus are duplicated/overlapping; either consolidate them into a single canonical key (e.g., connected) and update usages to reference it, or differentiate their copy text to reflect distinct UI contexts (e.g., keep connected for generic success, change connectedAfterSave to "Saved and connected to the Tuya cloud account." and keep connectedStatus as the short status "Connected to the Tuya cloud account."). Update all references in the codebase to use the chosen key/text and remove any redundant key to avoid maintenance drift.
1166-1167: Minor: Remove space before exclamation marks.The messages have a space before the exclamation mark ("with success !"), which is a French typographic convention. In English, there should be no space before the exclamation mark.
Suggested fix
"connected": "Connected to the Tuya cloud account with success !", - "connectedAfterSave": "Connected to the Tuya cloud account with success !", + "connected": "Connected to the Tuya cloud account with success!", + "connectedAfterSave": "Connected to the Tuya cloud account with success!",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front/src/config/i18n/en.json` around lines 1166 - 1167, Update the two i18n entries "connected" and "connectedAfterSave" to remove the extra space before the exclamation mark (change "with success !" to "with success!") so they follow English punctuation rules; edit the values for keys connected and connectedAfterSave in the en.json resource accordingly (no other logic changes).server/services/tuya/lib/tuya.localPoll.js (1)
26-31: Duplicate error listeners may cause confusion.Line 31 registers a persistent
onErrorlistener that storeslastErrorand logs, while line 69 registers a one-timeerrorListenerto reject the promise. Both fire on the same error event. The persistent listener on line 31 is never removed (only the one-time listener is removed in cleanup on line 46).While this doesn't cause a functional bug, consider removing the persistent listener in the final cleanup or consolidating the error handling.
Proposed fix: Remove persistent listener in final cleanup
} catch (e) { if (lastError && (!e || e.message !== lastError.message)) { logger.info(`[Tuya][localPoll] last socket error for device=${deviceId}: ${lastError.message}`); } logger.warn(`[Tuya][localPoll] failed for device=${deviceId}`, e); try { + tuyaLocal.removeListener('error', onError); await tuyaLocal.disconnect(); } catch (err) { // ignore } throw e; }Also add removal after successful completion around line 100:
logger.debug(`[Tuya][localPoll] device=${deviceId} dps=${JSON.stringify(data)}`); + tuyaLocal.removeListener('error', onError); return data;Also applies to: 65-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/services/tuya/lib/tuya.localPoll.js` around lines 26 - 31, The persistent error listener on tuyaLocal (onError which assigns lastError and logs) is registered but not removed, causing duplicate handlers alongside the one-time errorListener; fix by removing the persistent listener in the same cleanup path that removes errorListener and other handlers (use tuyaLocal.off/ removeListener for onError), or alternatively consolidate error handling by using only onError and removing the one-time errorListener registration (ensure you remove onError on both success and failure). Ensure you reference and remove the exact handler function name onError wherever you currently remove errorListener so no duplicate listeners remain.server/test/services/tuya/lib/tuya.localPoll.test.js (1)
128-159: Ensure fake timers are restored even on unexpected failures.If an assertion fails unexpectedly before reaching
clock.restore(), the fake timers will leak into subsequent tests. Usingtry/finallyensures cleanup regardless of how the test exits.Proposed fix using try/finally
it('should timeout', async () => { const clock = sinon.useFakeTimers(); - const connect = sinon.stub().resolves(); - const get = sinon.stub().returns(new Promise(() => {})); - const disconnect = sinon.stub().resolves(); - function TuyAPIStub() { - this.connect = connect; - this.get = get; - this.disconnect = disconnect; - } - const { localPoll } = proxyquire('../../../../services/tuya/lib/tuya.localPoll', { - tuyapi: TuyAPIStub, - }); - const promise = localPoll({ - deviceId: 'device', - ip: '1.1.1.1', - localKey: 'key', - protocolVersion: '3.3', - timeoutMs: 1000, - }); - await clock.tickAsync(1100); try { - await promise; - } catch (e) { - expect(e).to.be.instanceOf(BadParameters); - expect(e.message).to.equal('Local poll timeout'); - clock.restore(); - return; + const connect = sinon.stub().resolves(); + const get = sinon.stub().returns(new Promise(() => {})); + const disconnect = sinon.stub().resolves(); + function TuyAPIStub() { + this.connect = connect; + this.get = get; + this.disconnect = disconnect; + } + const { localPoll } = proxyquire('../../../../services/tuya/lib/tuya.localPoll', { + tuyapi: TuyAPIStub, + }); + const promise = localPoll({ + deviceId: 'device', + ip: '1.1.1.1', + localKey: 'key', + protocolVersion: '3.3', + timeoutMs: 1000, + }); + await clock.tickAsync(1100); + try { + await promise; + } catch (e) { + expect(e).to.be.instanceOf(BadParameters); + expect(e.message).to.equal('Local poll timeout'); + return; + } + throw new Error('Expected error'); + } finally { + clock.restore(); } - clock.restore(); - throw new Error('Expected error'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/test/services/tuya/lib/tuya.localPoll.test.js` around lines 128 - 159, The test 'should timeout' currently calls clock.restore() only on the success path and may leak fake timers if an assertion throws; wrap the promise awaiting and assertions in a try/catch/finally so clock.restore() is always called: after creating the promise and calling clock.tickAsync(1100) run the await promise inside a try where the success path throws 'Expected error', handle the expected BadParameters assertions in catch, and always call clock.restore() in the finally block; reference the test name 'should timeout', the Sinon fake timer variable 'clock', and the function under test 'localPoll' to locate the change.server/services/tuya/api/tuya.controller.js (1)
60-98: Consider extracting body parameters before callinglocalPoll.Line 61 calls
tuyaManager.localPoll(req.body)before extracting{ deviceId, ip, protocolVersion, localKey }on line 62. While this works, extracting parameters first would make the code flow clearer and allow for potential validation before the network call.Minor reorder suggestion
async function localPoll(req, res) { + const { deviceId, ip, protocolVersion, localKey } = req.body || {}; const result = await tuyaManager.localPoll(req.body); - const { deviceId, ip, protocolVersion, localKey } = req.body || {};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/services/tuya/api/tuya.controller.js` around lines 60 - 98, Extract req.body parameters (deviceId, ip, protocolVersion, localKey) before calling tuyaManager.localPoll so you can validate or mutate them prior to the network call; move the destructuring currently on line 62 up above the call to tuyaManager.localPoll(req.body) in the localPoll function, perform any lightweight validation/normalization on deviceId/ip/protocolVersion/localKey, then call tuyaManager.localPoll with the (possibly validated) body and continue the existing logic that updates tuyaManager.discoveredDevices and uses upsertParam.server/services/tuya/index.js (1)
48-53: Consider guarding interval setup against init failures.The reconnect interval is always started regardless of whether
init()succeeds. If initialization fails permanently (e.g., invalid credentials), the interval will keep attempting reconnection every 30 minutes. WhiletryReconnecthandles errors gracefully, this may generate unnecessary log noise and CPU cycles.Consider only starting the interval when
tuyaHandler.statusindicates a recoverable state, or add a failure counter to back off after repeated failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/services/tuya/index.js` around lines 48 - 53, The reconnect interval is being started unconditionally; change the logic so reconnectInterval is only created when the handler is in a recoverable state (e.g., check tuyaHandler.status against your STATUS enum and only set reconnectInterval when status is STATUS.CONNECTED, STATUS.DISCONNECTED, or STATUS.CONNECTING but NOT STATUS.FAILED), or alternatively initialize a failure counter/backoff inside tryReconnect and only start the interval after a successful init() or the first recoverable failure; update the block around tuyaHandler.status, reconnectInterval, tryReconnect and RECONNECT_INTERVAL_MS to perform this guard so permanent init failures don’t spawn endless intervals.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@front/src/config/i18n/de.json`:
- Around line 1219-1220: The "productKeyLabel" value is still English; update
the translation for the key "productKeyLabel" to German (e.g., change "Product
Key" to "Produktschlüssel") so it matches the localized "productIdLabel" and the
rest of the Tuya section.
In `@front/src/config/i18n/fr.json`:
- Around line 1349-1350: Translate the remaining English label "productKeyLabel"
to French and correct the pluralization in the related "notConfigured" message:
replace the value of productKeyLabel (currently "Product Key") with an
appropriate French phrase (e.g., "Clé produit") and update the "notConfigured"
string so it uses the plural form of "informations" or rephrase to a
grammatically correct French sentence (e.g., "Aucune information configurée" or
"Informations non configurées") where the key name is "notConfigured" (also
ensure identical fixes are applied at the other occurrence around line 1400).
In `@front/src/routes/integration/all/tuya/TuyaDeviceBox.jsx`:
- Around line 111-170: The pollLocal flow sets device from response.device
earlier but later reconstructs device using this.state.device and old params,
which can drop fields from the poll response; update the final setState to base
the new device on the most recent updatedDevice (if present) instead of
this.state.device and merge/replace its params with newParams (using symbols:
pollLocal, response.device/updatedDevice, this.state.device, params/newParams,
usedProtocol) so the polled payload fields are preserved while only updating
PROTOCOL_VERSION.
- Around line 181-198: In updateIpAddress, the CLOUD_IP param lookup currently
falls back to undefined so overrideValue can be wrong when the backend omitted
the param; change the cloudIp assignment to use the device's cloud_ip fallback
(e.g., set cloudIp = cloudIpParam ? cloudIpParam.value :
this.state.device.cloud_ip) so LOCAL_OVERRIDE logic (and params updates for
'LOCAL_OVERRIDE' and 'IP_ADDRESS') correctly reflects differences between the
entered IP and the device.cloud_ip.
In `@server/services/tuya/lib/device/tuya.convertDevice.js`:
- Around line 57-60: The current logger.debug(JSON.stringify(tuyaDevice)) call
in tuya.convertDevice.js exposes sensitive fields (e.g., local_key, token, ip,
gwId); replace it with a redacted summary by building an object from tuyaDevice
that only includes safe identifiers (e.g., id, name, productName/model,
category, online status) and omits or masks sensitive keys (specifically
remove/mask local_key, token, ip, gwId, and any credential fields) before
logging; update the logging around the tuyaDevice variable and the nearby
logger.debug that references name/productName to use this redactedSummary so
only non-sensitive info is written to logs.
In `@server/services/tuya/lib/tuya.loadDeviceDetails.js`:
- Around line 15-31: The debug logs in tuya.loadDeviceDetails.js currently dump
raw device payloads (variables specifications and details) which may contain
secrets like local_key and IPs; update the logging in the loadDeviceDetails flow
to stop printing full JSON — either remove these logger.debug calls or replace
them with a redacted summary that omits sensitive fields (e.g., local_key,
token, ip, gwId) and only logs non-sensitive identifiers (deviceId, model,
productName) or a boolean flag indicating presence of those fields; locate the
two logger.debug lines referring to specifications and details and change them
to redact or remove raw payload output.
In `@server/services/tuya/lib/tuya.localScan.js`:
- Around line 70-83: The payload sometimes omits payload.ip so local discovery
should fall back to the sender address; update the localScan handling that sets
devices[deviceId] so the ip stored is payload.ip when present, otherwise use
rinfo.address (or the existing remote variable) as a fallback. Locate the block
that computes deviceId and assigns devices[deviceId] and change the ip
assignment to use the fallback (keeping version and productKey unchanged) so
missing payload.ip no longer results in an empty IP for the device.
- Around line 51-59: The info-level logging in tuya.localScan.js currently
prints raw packet hex/ascii and the full parsed object (logger.info +
formatHex/formatAscii + JSON.stringify(parsed)), which can expose device data;
change those logger.info calls to logger.debug and remove or redact raw payloads
and full parsed output—log only a minimal summary (e.g., remote/source and
byteLen and a redacted marker or select safe fields) and avoid printing
parsed[0].payload or full JSON; update the log lines around parser.parse,
formatHex, formatAscii, and the parsed payload assignment to use debug/redaction
instead of info/full dumps.
In `@server/services/tuya/lib/tuya.setValue.js`:
- Around line 73-88: The local Tuya path may connect but skip disconnect if
tuyaLocal.set throws, leaking sockets; modify the block that creates TuyAPI and
calls tuyaLocal.connect()/set()/disconnect() (references: hasLocalConfig,
localDps, new TuyAPI, tuyaLocal.connect, tuyaLocal.set, tuyaLocal.disconnect) to
ensure disconnect is always invoked by using a try/finally (or ensuring
disconnect in the catch) so that tuyaLocal.disconnect() runs whether set()
succeeds or throws, and keep the existing logger.warn fallback to cloud on
error.
In `@server/test/services/tuya/lib/controllers/tuya.controller.test.js`:
- Around line 135-149: The test reassigns tuyaManager.localScan directly which
pollutes global state across tests; instead, change the test to stub/spy and
restore the original between tests by using sinon.stub/replace (or saving the
original function and restoring it in beforeEach/afterEach) for
tuyaManager.localScan so the fake does not persist—update the test that calls
controller['post /api/v1/service/tuya/local-scan'].controller to use a
sinon.stub/resolves for tuyaManager.localScan and ensure the original is
restored in the test harness (beforeEach/afterEach) so subsequent tests see the
real implementation and tuyaManager.discoveredDevices remains isolated.
---
Outside diff comments:
In `@front/src/routes/integration/all/tuya/setup-page/SetupTab.jsx`:
- Around line 144-179: The configured check uses raw state while you trim values
before saving, so create trimmed local variables (e.g., const tuyaAccessKey =
this.state.tuyaAccessKey.trim(), const tuyaSecretKey =
this.state.tuyaSecretKey.trim(), and trim tuyaAppAccountId and tuyaAppUsername
similarly), use those trimmed variables in the httpClient.post calls (instead of
this.state.*.trim() inline) and compute configured with the trimmed credentials
(e.g., const configured = !!(this.state.tuyaEndpoint && tuyaAccessKey &&
tuyaSecretKey)); update any subsequent logic that relies on the values to use
the trimmed variables.
---
Nitpick comments:
In `@front/src/config/i18n/en.json`:
- Line 1121: The "scanInProgress" i18n message currently reads "Scan in
progress... retrieving cloud devices." but the scan now covers both cloud and
local devices; update the translation for the "scanInProgress" key to a neutral
phrase such as "Scan in progress..." or "Scan in progress... retrieving
devices." so it accurately reflects both cloud and local scans—locate the
"scanInProgress" entry in en.json and replace the string accordingly.
- Around line 1166-1168: The three i18n keys connected, connectedAfterSave, and
connectedStatus are duplicated/overlapping; either consolidate them into a
single canonical key (e.g., connected) and update usages to reference it, or
differentiate their copy text to reflect distinct UI contexts (e.g., keep
connected for generic success, change connectedAfterSave to "Saved and connected
to the Tuya cloud account." and keep connectedStatus as the short status
"Connected to the Tuya cloud account."). Update all references in the codebase
to use the chosen key/text and remove any redundant key to avoid maintenance
drift.
- Around line 1166-1167: Update the two i18n entries "connected" and
"connectedAfterSave" to remove the extra space before the exclamation mark
(change "with success !" to "with success!") so they follow English punctuation
rules; edit the values for keys connected and connectedAfterSave in the en.json
resource accordingly (no other logic changes).
In `@server/services/tuya/api/tuya.controller.js`:
- Around line 60-98: Extract req.body parameters (deviceId, ip, protocolVersion,
localKey) before calling tuyaManager.localPoll so you can validate or mutate
them prior to the network call; move the destructuring currently on line 62 up
above the call to tuyaManager.localPoll(req.body) in the localPoll function,
perform any lightweight validation/normalization on
deviceId/ip/protocolVersion/localKey, then call tuyaManager.localPoll with the
(possibly validated) body and continue the existing logic that updates
tuyaManager.discoveredDevices and uses upsertParam.
In `@server/services/tuya/index.js`:
- Around line 48-53: The reconnect interval is being started unconditionally;
change the logic so reconnectInterval is only created when the handler is in a
recoverable state (e.g., check tuyaHandler.status against your STATUS enum and
only set reconnectInterval when status is STATUS.CONNECTED, STATUS.DISCONNECTED,
or STATUS.CONNECTING but NOT STATUS.FAILED), or alternatively initialize a
failure counter/backoff inside tryReconnect and only start the interval after a
successful init() or the first recoverable failure; update the block around
tuyaHandler.status, reconnectInterval, tryReconnect and RECONNECT_INTERVAL_MS to
perform this guard so permanent init failures don’t spawn endless intervals.
In `@server/services/tuya/lib/tuya.localPoll.js`:
- Around line 26-31: The persistent error listener on tuyaLocal (onError which
assigns lastError and logs) is registered but not removed, causing duplicate
handlers alongside the one-time errorListener; fix by removing the persistent
listener in the same cleanup path that removes errorListener and other handlers
(use tuyaLocal.off/ removeListener for onError), or alternatively consolidate
error handling by using only onError and removing the one-time errorListener
registration (ensure you remove onError on both success and failure). Ensure you
reference and remove the exact handler function name onError wherever you
currently remove errorListener so no duplicate listeners remain.
In `@server/test/services/tuya/lib/tuya.localPoll.test.js`:
- Around line 128-159: The test 'should timeout' currently calls clock.restore()
only on the success path and may leak fake timers if an assertion throws; wrap
the promise awaiting and assertions in a try/catch/finally so clock.restore() is
always called: after creating the promise and calling clock.tickAsync(1100) run
the await promise inside a try where the success path throws 'Expected error',
handle the expected BadParameters assertions in catch, and always call
clock.restore() in the finally block; reference the test name 'should timeout',
the Sinon fake timer variable 'clock', and the function under test 'localPoll'
to locate the change.
|
@coderabbitai full review Please review the full PR again, but keep feedback limited to the actual scope of this PR and the files impacted by it. Focus on concrete regressions, inconsistencies with existing Gladys patterns, and real risks for current behavior. Avoid speculative improvements, overengineering, or suggestions unrelated to the current implementation goals. Please review this PR with these priorities: Compare the code style and patterns to the existing Gladys codebase and avoid suggestions that diverge from current project habits. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll gather the actual code to conduct a thorough, focused review. 🏁 Script executed: Length of output: 29598 🏁 Script executed: Length of output: 17052 🏁 Script executed: Length of output: 35849 🏁 Script executed: Length of output: 29111 🏁 Script executed: Length of output: 3675
🔴 Concrete Bugs / Regressions1.
|
- Rename protocol35Unsupported i18n key to protocolVersionRequired with informational message and block 3.5 selection in JS handler - Remove redundant disconnect() call in localPoll outer catch - Remove !tuyaConfigured from disconnect button disabled condition - Expose saveConfiguration via POST /api/v1/service/tuya/configuration and simplify frontend to use single endpoint (align with Gladys pattern) - Add appAccountId to connect() guard for consistency with getStatus() - Update connect test for new guard behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Pierre-Gilles
left a comment
Pierre-Gilles
left a comment
There was a problem hiding this comment.
Thanks for the PR 🙌 I really think this will help Tuya users discover Gladys!
Quick question: did you build this PR with Claude Code? If so, which model did you use? 🙂
cloudTitle and localTitle i18n keys now contain only plain text. The <strong> and <br /> markup is handled in SetupTab.jsx. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When Tuya credentials are missing, init() now returns immediately with NOT_INITIALIZED status instead of attempting a connection. This avoids unnecessary errors at startup for users who do not use the Tuya integration. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ect.js Extract tryReconnect, scheduleQuickReconnects and clearQuickReconnects into a dedicated module with a createReconnectManager() factory. index.js now follows the standard Gladys service structure. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Thanks! Yes of course, I started the development with Codex GPT-5.2, then continued with GPT-5.3-Max, and finally did a full review pass with Claude Opus 4.6 (Claude Code). |
- Move reconnect logic into TuyaHandler prototype methods instead of factory pattern, following standard Gladys service conventions - Use ServiceNotConfiguredError in init() when credentials are missing instead of silent return, matching existing Gladys patterns - Normalize localScan parameter to object-only type for consistency Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Call addSelector() after building device/feature objects, matching the pattern used by zigbee2mqtt and tasmota. Without this, updating a device from the discovery page overwrites the slugified selector in DB with the raw value (e.g. tuya:id:code instead of tuya-id-code), breaking dashboard widget references. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
server/services/tuya/index.js
Outdated
| if (tuyaHandler.status === STATUS.CONNECTED) { | ||
| await tuyaHandler.loadDevices(); | ||
| } | ||
| tuyaHandler.startReconnect(); |
There was a problem hiding this comment.
Why isn’t all of this handled in tuyaHandler.init()?
In my opinion, tuyaHandler should behave like a small, self-contained library and be responsible for its own logic.
index.js should remain a minimal bridge between the Gladys core and the integration.
There was a problem hiding this comment.
The reasoning behind having loadDevices and startReconnect in index.js was separation of concerns: init() handles configuration and connection, while the reconnect mechanism is a layer above that monitors and restarts — a different responsibility. Having it in index.js made the startup orchestration explicitly visible at a glance: init → load devices if connected → start reconnect watcher. We also wanted to avoid startReconnect() being called on every init() invocation if it were ever reused internally (e.g. during reconnect flows).
That said, I understand the convention — the handler should be a self-contained library and index.js a minimal bridge, like netatmo. Moved loadDevices() and startReconnect() into init().
server/services/tuya/index.js
Outdated
| */ | ||
| async function stop() { | ||
| logger.info('Stopping Tuya service'); | ||
| tuyaHandler.stopReconnect(); |
There was a problem hiding this comment.
Why isn't this done in disconnect?
There was a problem hiding this comment.
Same logic here: disconnect() is responsible for cleaning up the connection state (nullify connector, reset status, notify via websocket). stopReconnect() is about stopping the monitoring timers — a separate concern. Keeping them distinct in index.js made the shutdown sequence readable: stop reconnect watcher → then disconnect.
Aligned with the convention — stopReconnect() is now called inside disconnect().
…nect index.js is now a minimal bridge (like zigbee2mqtt): start() calls init(), stop() calls disconnect(). All orchestration lives inside the handler where it belongs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Pierre-Gilles
left a comment
Pierre-Gilles
left a comment
There was a problem hiding this comment.
Great PR! 🙌
Just one more tester and we should be good to go. Thanks a lot for the fixes!
2 participants
Pull Request check-list
To ensure your Pull Request can be accepted as fast as possible, make sure to review and check all of these items:
npm teston both front/server)npm run eslinton both front/server)npm run prettieron both front/server)npm run compare-translationson front)front/src/config/demo.js) so that the demo website is working without a backend? (if needed) See https://demo.gladysassistant.com.NOTE: these things are not required to open a PR and can be done afterwards / while the PR is open.
Description of change
Scope
Translate
Front
Server
Server Tests
Package
Before
After
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests