Fix/decoder substring matching - #1057
Draft
jona159 wants to merge 7 commits into
Draft
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/db/models/device.server.ts (1)
1095-1112: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winValidate a caller-supplied
sensorDefinitionIdon custom devices.The last branch stores
sensorData.dataunchanged. A client that creates a custom device can therefore setdata.sensorDefinitionIdto any catalog key.findLuftdatenSensorMappingaccepts that key and applies its mapping, including the0.01pressure multiplier, so stored measurement values change silently.Either strip unknown
sensorDefinitionIdvalues in this branch, or validate the key againstsensorDefinitionsbefore persisting it.♻️ Proposed validation
+ const requestedDefinitionId = existingSensorData.sensorDefinitionId + const isKnownDefinitionId = + typeof requestedDefinitionId === 'string' && + requestedDefinitionId in sensorDefinitions const sensorMetadata = storedDeviceSchemaVersion ? { ...existingSensorData, deviceSchemaSensorId: sensorData.id, } : usesSensorDefinitions ? { ...existingSensorData, sensorDefinitionId: sensorData.id, } - : sensorData.data + : requestedDefinitionId !== undefined && !isKnownDefinitionId + ? { ...existingSensorData, sensorDefinitionId: undefined } + : sensorData.data🤖 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 `@app/db/models/device.server.ts` around lines 1095 - 1112, Validate the caller-supplied sensorDefinitionId in the final branch of the sensorMetadata construction before persisting sensorData.data. Accept it only when it matches a key in sensorDefinitions; otherwise remove or ignore that field while preserving the rest of the custom sensor data. Keep the storedDeviceSchemaVersion and usesSensorDefinitions branches unchanged.tests/services/decoding-service.server.spec.ts (1)
105-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for mixed sensor sets and duplicate catalog value types.
Two paths stay untested. First, a device that has both catalog sensors and legacy title-only sensors in one request. Second, a device that has two sensors whose definitions claim the same Luftdaten value type, for example
pms5003_pm01andpms7003_pm01. The second case is the failure described inapp/lib/sensor-definitions.ts. A test would pin the intended behaviour.🤖 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 `@tests/services/decoding-service.server.spec.ts` around lines 105 - 159, Extend the decodeMeasurements test suite with coverage for a request containing both catalog-defined sensors and legacy title-only sensors, asserting each maps correctly. Add a separate test with sensor definitions such as pms5003_pm01 and pms7003_pm01 that resolve to the same Luftdaten value type, and assert the intended duplicate-mapping behavior described by sensor-definitions.ts.
🤖 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 `@app/lib/sensor-definitions.ts`:
- Around line 417-437: Update the Luftdaten entries in sensorDefinitions so
findLuftdatenSensorMapping can resolve each affected valueType—PMS_P0, PMS_P1,
PMS_P2, BME280_pressure, BMP180_pressure, temperature, and humidity—without
ambiguity when all model sensors are added. Ensure each value type has exactly
one mapping, or add deterministic discriminator metadata such as sensor type or
unit, while preserving the intended sensor phenomena.
In `@app/services/decoding-service.server.ts`:
- Around line 90-122: Change the ambiguity handling across
app/services/decoding-service.server.ts:90-122, :188-211, and :352-383 so one
unresolved value is skipped without aborting the full upload. In the Luftdaten
resolver at :90-122 and findHackairSensorId at :188-211, return undefined or
record the conflict instead of throwing; in the duplicate
destination-measurement handling at :352-383, drop the later duplicate after
this non-throwing policy is applied, preserving other measurements in the
request.
---
Nitpick comments:
In `@app/db/models/device.server.ts`:
- Around line 1095-1112: Validate the caller-supplied sensorDefinitionId in the
final branch of the sensorMetadata construction before persisting
sensorData.data. Accept it only when it matches a key in sensorDefinitions;
otherwise remove or ignore that field while preserving the rest of the custom
sensor data. Keep the storedDeviceSchemaVersion and usesSensorDefinitions
branches unchanged.
In `@tests/services/decoding-service.server.spec.ts`:
- Around line 105-159: Extend the decodeMeasurements test suite with coverage
for a request containing both catalog-defined sensors and legacy title-only
sensors, asserting each maps correctly. Add a separate test with sensor
definitions such as pms5003_pm01 and pms7003_pm01 that resolve to the same
Luftdaten value type, and assert the intended duplicate-mapping behavior
described by sensor-definitions.ts.
🪄 Autofix
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 Plus
Run ID: b15712b9-478f-40a6-887c-b42e50b19399
📒 Files selected for processing (7)
app/db/models/device.server.tsapp/lib/model-definitions.tsapp/lib/sensor-definitions.tsapp/services/decoding-service.server.tstests/db/models/device.server.spec.tstests/services/decoding-service.server.spec.tstests/services/hackair-decoding-service.server.spec.ts
Comment on lines
+417
to
+437
| pms5003_pm01: { | ||
| phenomenon: 'particulate-matter-mass-concentration-1um', | ||
| decoderMappings: { luftdaten: [{ valueType: 'PMS_P0' }] }, | ||
| }, | ||
| pms5003_pm25: { | ||
| phenomenon: 'particulate-matter-mass-concentration-2.5um', | ||
| decoderMappings: { luftdaten: [{ valueType: 'PMS_P2' }] }, | ||
| }, | ||
| bme280_pressure_pa: { | ||
| phenomenon: 'atmospheric-pressure', | ||
| decoderMappings: { luftdaten: [{ valueType: 'BME280_pressure' }] }, | ||
| }, | ||
| bme680_humidity: { phenomenon: 'relative-humidity' }, | ||
| bme280_humidity: { | ||
| phenomenon: 'relative-humidity', | ||
| decoderMappings: { luftdaten: [{ valueType: 'BME280_humidity' }] }, | ||
| }, | ||
| pms5003_pm10: { | ||
| phenomenon: 'particulate-matter-mass-concentration-10um', | ||
| decoderMappings: { luftdaten: [{ valueType: 'PMS_P1' }] }, | ||
| }, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Detect duplicate Luftdaten value types within each model definition.
set -euo pipefail
fd -t f 'sensor-definitions.ts' app/lib
fd -t f 'model-definitions.ts' app/lib
python3 - <<'PY'
import re, pathlib
sd = pathlib.Path('app/lib/sensor-definitions.ts').read_text()
md = pathlib.Path('app/lib/model-definitions.ts').read_text()
# metadata block only
meta = sd.split('const sensorDefinitionMetadata', 1)[1]
mapping = {}
for m in re.finditer(r"(\w+):\s*\{(.*?)\n\t\},", meta, re.S):
key, body = m.group(1), m.group(2)
vts = re.findall(r"valueType:\s*'([^']+)'", body)
if vts:
mapping[key] = vts
for model in re.finditer(r"'?([\w.:]+)'?:\s*\[(.*?)\]\s*as const", md, re.S):
name, body = model.group(1), model.group(2)
keys = re.findall(r"'([\w]+)'", body)
seen = {}
for k in keys:
for vt in mapping.get(k, []):
seen.setdefault(vt, []).append(k)
dups = {vt: ks for vt, ks in seen.items() if len(ks) > 1}
if dups:
print(f"MODEL {name}")
for vt, ks in dups.items():
print(f" {vt}: {ks}")
PYRepository: openSenseMap/frontend
Length of output: 589
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- relevant model and sensor definitions ---'
rg -n -C 8 "luftdaten\.info|pms5003_pm01|pms7003_pm01|pms1003_pm01|pms3003_pm01|pms5003_pm10|pms3003_pm10|pms1003_pm10|pms3003_pm25|pms5003_pm25|bmp180_pressure_(pa|hpa)|bme280_pressure_(pa|hpa)|dht(11|22)" app/lib/sensor-definitions.ts app/lib/model-definitions.ts
printf '%s\n' '--- decoder mapping implementation and call sites ---'
rg -n -C 12 "findLuftdatenSensorMapping|Ambiguous Luftdaten sensor definition mapping|decoderMappings" app/services app/libRepository: openSenseMap/frontend
Length of output: 45228
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- sensor creation and definition-ID resolution ---'
rg -n -C 10 "sensorDefinitionId|sensorTemplates|modelDefinitions|model-definitions|sensorDefinitions" app --glob '!lib/sensor-definitions.ts' --glob '!lib/model-definitions.ts'
printf '%s\n' '--- decoder tests and upload error handling ---'
rg -n -C 12 "findLuftdatenSensorMapping|Ambiguous Luftdaten|decodeMessage|luftdaten" app --glob '*test*' --glob '*spec*' --glob '*.ts' --glob '*.tsx'
printf '%s\n' '--- exact model block ---'
sed -n '55,102p' app/lib/model-definitions.ts
printf '%s\n' '--- exact decoder implementation ---'
sed -n '100,175p' app/services/decoding-service.server.ts
sed -n '335,385p' app/services/decoding-service.server.tsRepository: openSenseMap/frontend
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import re
from collections import defaultdict
from pathlib import Path
sensor_text = Path("app/lib/sensor-definitions.ts").read_text()
model_text = Path("app/lib/model-definitions.ts").read_text()
decoder_text = Path("app/services/decoding-service.server.ts").read_text()
metadata = sensor_text.split("const sensorDefinitionMetadata = {", 1)[1].split(
"} as const satisfies", 1
)[0]
definitions = {}
starts = list(re.finditer(r"(?m)^\t([A-Za-z0-9_]+):\s*\{", metadata))
for index, match in enumerate(starts):
end = starts[index + 1].start() if index + 1 < len(starts) else len(metadata)
body = metadata[match.start():end]
definitions[match.group(1)] = re.findall(
r"valueType:\s*'([^']+)'", body
)
model_match = re.search(
r"'luftdaten\.info':\s*\[(.*?)\]\s*as const", model_text, re.S
)
assert model_match, "luftdaten.info model was not found"
model_keys = re.findall(r"'([A-Za-z0-9_]+)'", model_match.group(1))
claims = defaultdict(list)
for key in model_keys:
for value_type in definitions.get(key, []):
claims[value_type.lower()].append(key)
duplicates = {
value_type: keys for value_type, keys in claims.items() if len(keys) > 1
}
print("Duplicate definition mappings in luftdaten.info:")
for value_type, keys in sorted(duplicates.items()):
print(f" {value_type}: {keys}")
assert all(
key in duplicates
for key in (
"pms_p0",
"pms_p1",
"pms_p2",
"bme280_pressure",
"bmp180_pressure",
"temperature",
"humidity",
)
)
assert "if (definitionMatches.length > 1)" in decoder_text
print("\nThe decoder throws when more than one sensor matches a value type.")
# Emulate the definition-matching branch for a default model device.
for value_type, keys in sorted(duplicates.items()):
matches = [
{"id": key, "data": {"sensorDefinitionId": key}}
for key in model_keys
if key in keys
]
assert len(matches) > 1
print(f" {value_type}: {len(matches)} matches")
PYRepository: openSenseMap/frontend
Length of output: 903
Prevent ambiguous Luftdaten mappings
luftdaten.info contains multiple definitions for PMS_P0, PMS_P1, PMS_P2, BME280_pressure, BMP180_pressure, temperature, and humidity. When sensorTemplates is omitted, device creation adds all model sensors. findLuftdatenSensorMapping then throws Ambiguous Luftdaten sensor definition mapping ..., so an upload containing any affected value type fails. Enforce one mapping per value type or add a deterministic discriminator such as sensor type or unit.
🤖 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 `@app/lib/sensor-definitions.ts` around lines 417 - 437, Update the Luftdaten
entries in sensorDefinitions so findLuftdatenSensorMapping can resolve each
affected valueType—PMS_P0, PMS_P1, PMS_P2, BME280_pressure, BMP180_pressure,
temperature, and humidity—without ambiguity when all model sensors are added.
Ensure each value type has exactly one mapping, or add deterministic
discriminator metadata such as sensor type or unit, while preserving the
intended sensor phenomena.
Comment on lines
+90
to
+122
| const aliases = luftdatenMatchings[vt_phenomenon] | ||
| const compatibleSensors = sensors.filter((sensor) => { | ||
| if (!sensor?.id || !sensor.title) return false | ||
| if (!sensor.sensorType) return true | ||
|
|
||
| const title = sensor.title.toLowerCase() | ||
| return sensor.sensorType.toLowerCase().startsWith(vt_sensortype) | ||
| }) | ||
|
|
||
| if (sensor.sensorType) { | ||
| const type = sensor.sensorType.toLowerCase() | ||
| if (!type.startsWith(vt_sensortype)) continue | ||
| } | ||
| const exactMatches = compatibleSensors.filter((sensor) => { | ||
| const title = sensor.title!.toLowerCase() | ||
| return title === vt_phenomenon || aliases.includes(title) | ||
| }) | ||
|
|
||
| const aliases = luftdatenMatchings[vt_phenomenon] | ||
| const titleMatches = | ||
| title === vt_phenomenon || | ||
| aliases.includes(title) || | ||
| aliases.some((alias) => title.includes(alias)) | ||
| if (exactMatches.length > 1) { | ||
| throw new Error( | ||
| `Ambiguous Luftdaten sensor mapping for value type ${value_type}`, | ||
| ) | ||
| } | ||
| if (exactMatches.length === 1) return exactMatches[0].id | ||
|
|
||
| if (titleMatches) return sensor.id | ||
| const substringMatches = compatibleSensors.filter((sensor) => { | ||
| const title = sensor.title!.toLowerCase() | ||
| return aliases.some((alias) => title.includes(alias)) | ||
| }) | ||
|
|
||
| if (substringMatches.length > 1) { | ||
| throw new Error( | ||
| `Ambiguous Luftdaten sensor mapping for value type ${value_type}`, | ||
| ) | ||
| } | ||
|
|
||
| return undefined | ||
| return substringMatches[0]?.id | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Ambiguous or duplicated mappings reject the complete upload. All three sites throw instead of skipping the affected value, so one unresolvable reading discards every other measurement in the same request. Existing devices with duplicate sensor titles, and new model devices with duplicate catalog value types, lose all data of each upload.
app/services/decoding-service.server.ts#L90-L122: returnundefinedfor an ambiguous Luftdaten title match, or record the conflict, instead of throwing out ofdecodeMessage.app/services/decoding-service.server.ts#L188-L211: apply the same non-throwing resolution infindHackairSensorId.app/services/decoding-service.server.ts#L375-L383: drop the later duplicate destination measurement, or keep the throw only after the ambiguity policy above is settled.
📍 Affects 1 file
app/services/decoding-service.server.ts#L90-L122(this comment)app/services/decoding-service.server.ts#L188-L211app/services/decoding-service.server.ts#L352-L383
🤖 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 `@app/services/decoding-service.server.ts` around lines 90 - 122, Change the
ambiguity handling across app/services/decoding-service.server.ts:90-122,
:188-211, and :352-383 so one unresolved value is skipped without aborting the
full upload. In the Luftdaten resolver at :90-122 and findHackairSensorId at
:188-211, return undefined or record the conflict instead of throwing; in the
duplicate destination-measurement handling at :352-383, drop the later duplicate
after this non-throwing policy is applied, preserving other measurements in the
request.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Type of Change
Implementation
Checklist
devbranchAdditional Information
Summary by CodeRabbit
New Features
Bug Fixes