Telemetry UI - #1361
Conversation
|
Someone is attempting to deploy a commit to the Meshtastic Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds node-metric persistence and recording, a telemetry page with D3 charts and raw readings, telemetry navigation and routing, plus a Bluetooth configuration form. ChangesTelemetry metrics flow
Bluetooth configuration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TelemetryPage
participant MeshClient
participant SqlocalNodeMetricsRepository
participant TelemetryChart
TelemetryPage->>MeshClient: read telemetry history
TelemetryPage->>SqlocalNodeMetricsRepository: load recent node metrics
TelemetryPage->>TelemetryChart: provide selected series
TelemetryChart-->>TelemetryPage: render chart and tooltip
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🔧 Fix failing CI
⚔️ Resolve merge conflicts
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
apps/web/package.json (1)
68-68: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
@types/d3todevDependencies.Type-only package; it ships nothing at runtime and inflates the production dependency set.
♻️ Suggested change
- "`@types/d3`": "^7.4.3",Then add it under
devDependencies:"`@types/d3`": "^7.4.3",🤖 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 `@apps/web/package.json` at line 68, Move the `@types/d3` entry from dependencies to devDependencies in the package manifest, preserving its version constraint and ensuring it appears only once.apps/web/src/pages/Telemetry/statNames.ts (1)
135-137: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueObject-literal lookup can return inherited members.
Keys come from DB
metricvalues / decoded payload fields; a key likeconstructorortoStringresolves through the prototype chain and??won't catch it, so a non-string is returned. UseObject.hasOwnor aMap.🛡️ Suggested change
export function translateStatName(name: string): string { - return STAT_DISPLAY_NAMES[name] ?? name; + return Object.hasOwn(STAT_DISPLAY_NAMES, name) + ? STAT_DISPLAY_NAMES[name] + : name; }🤖 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 `@apps/web/src/pages/Telemetry/statNames.ts` around lines 135 - 137, Update translateStatName to restrict STAT_DISPLAY_NAMES lookups to own properties, using Object.hasOwn before returning the mapped value; otherwise return the original name. Preserve the existing fallback behavior for unknown metric names, including prototype keys such as constructor and toString.apps/web/src/pages/Telemetry/index.tsx (4)
529-542: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRaw-row string is built on every render even while the
<details>is collapsed.
readingscan hold thousands of rows; themap/JSON.stringify/joinruns on each render regardless of whether the section is open. Memoize it and gate on the open state.♻️ Suggested change
+ const [rawOpen, setRawOpen] = useState(false); + const rawText = useMemo( + () => + readings.length === 0 + ? "No telemetry stored for this node yet." + : readings + .map( + (r) => + `${r.time.toISOString()} ${r.kind}\n${JSON.stringify( + r.value, + jsonReplacer, + 2, + )}`, + ) + .join("\n\n"), + [readings], + );- <details className="mt-6"> + <details + className="mt-6" + open={rawOpen} + onToggle={(e) => setRawOpen(e.currentTarget.open)} + ><pre className="text-xs bg-slate-900 text-slate-100 p-4 rounded-lg overflow-auto max-h-96 mt-2"> - {readings.length === 0 - ? "No telemetry stored for this node yet." - : readings - .map(...) - .join("\n\n")} + {rawOpen ? rawText : null} </pre>🤖 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 `@apps/web/src/pages/Telemetry/index.tsx` around lines 529 - 542, Update the telemetry details rendering around the readings display to memoize the raw-row string and only compute it when the `<details>` section is open. Reuse the memoized value in the `<pre>` output while preserving the existing empty-state message and formatting for non-empty readings.
474-528: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew Telemetry UI strings bypass i18n. Both new surfaces hardcode English while the surrounding app resolves copy through
useTranslation, so the Telemetry feature stays untranslated in every locale.
apps/web/src/pages/Telemetry/index.tsx#L474-L528: route "Telemetry Data", "Select Node", "Select Stat", the two select placeholders, and the raw-rows header throught(...)with new translation keys.apps/web/src/components/Sidebar.tsx#L135-L139: replace the literal"Telemetry"witht("navigation.telemetry")and add the key to theuinamespace resources.🤖 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 `@apps/web/src/pages/Telemetry/index.tsx` around lines 474 - 528, Update apps/web/src/pages/Telemetry/index.tsx lines 474-528 to obtain useTranslation and route “Telemetry Data,” “Select Node,” “Select Stat,” both select placeholders, and the raw-rows header through t(...) using new translation keys. Update apps/web/src/components/Sidebar.tsx lines 135-139 to use t("navigation.telemetry") instead of the literal label, and add that key to the ui namespace resources.
264-268: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valuePrefer composed elements over
.html()for the tooltip.
labeloriginates fromnode_metrics.metric/ decoded payload keys, so it isn't a trusted literal in all paths. Building nodes with.text()removes the injection surface entirely without changing appearance.🤖 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 `@apps/web/src/pages/Telemetry/index.tsx` around lines 264 - 268, Replace the tooltip construction in the chart callback using .html() with composed DOM elements and .text() for label, valueFmt(point.value), and dateFmt(new Date(point.time)). Preserve the existing styling, element order, and visual appearance while ensuring decoded metric labels are rendered as text rather than interpreted as HTML.
44-59: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winChart won't re-measure when the container resizes without a window resize.
Sidebar collapse/expand and layout changes resize the container without firing
resize, so the SVG keeps drawing at stale width (noviewBox, so content is clipped or leaves dead space). AResizeObservercovers both cases.♻️ Suggested change
- handleResize(); - window.addEventListener("resize", handleResize); - return () => window.removeEventListener("resize", handleResize); + handleResize(); + const el = containerRef.current; + if (!el) return; + const observer = new ResizeObserver(handleResize); + observer.observe(el); + return () => observer.disconnect();🤖 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 `@apps/web/src/pages/Telemetry/index.tsx` around lines 44 - 59, Update the dimensions measurement effect in the Telemetry page to observe containerRef.current with a ResizeObserver, invoking the existing handleResize whenever the container changes size. Keep the initial measurement and cleanup behavior, and disconnect the observer during effect cleanup while retaining the window resize listener if needed.apps/web/src/routes.tsx (1)
7-7: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winLazy-load the telemetry route.
TelemetryPageimportsd3directly, but the route component is eagerly referenced inapps/web/src/routes.tsx:151. Use a code-split loader here (lazyRouteComponent(...),lazy: ..., orReact.lazy) so the chart library isn’t included until/telemetryis opened.🤖 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 `@apps/web/src/routes.tsx` at line 7, Update the telemetry route configuration and its TelemetryPage import so the page is loaded through the project’s lazy route mechanism (such as lazyRouteComponent, a lazy route loader, or React.lazy) rather than eagerly referencing the component. Preserve the existing /telemetry behavior while deferring the d3-containing TelemetryPage module until that route is opened.
🤖 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 `@apps/web/src/components/PageComponents/Telemetry/Battery.tsx`:
- Around line 51-81: Update BluetoothValidationSchema so fixedPin is optional
for RANDOM_PIN and NO_PIN but required as a six-digit PIN when mode is
FIXED_PIN, while preserving the existing Bluetooth validation rules. Apply the
same conditional schema and UI behavior to the Bluetooth Settings form, using
the mode field and fixedPin configuration symbols already shared by the
Bluetooth forms.
In `@apps/web/src/pages/Telemetry/index.tsx`:
- Around line 342-350: Update nodeDisplayName to format node.num as exactly 8
lowercase hexadecimal characters, padding leading zeros as needed; use this
canonical value for both the fallback name’s hex suffix and the returned !node
ID while preserving the existing uppercase presentation of the fallback suffix.
In
`@packages/sdk-storage-sqlocal/src/nodeMetrics/SqlocalNodeMetricsRepository.ts`:
- Around line 102-128: Validate policy.maxPerMetric before using it to derive
the query offset in the retention logic: accept only a finite, non-negative
integer, or explicitly implement and preserve the documented behavior that zero
clears each bucket. Reject invalid values such as negatives, fractions, and
Infinity before the overCap query and pruning loop run.
---
Nitpick comments:
In `@apps/web/package.json`:
- Line 68: Move the `@types/d3` entry from dependencies to devDependencies in the
package manifest, preserving its version constraint and ensuring it appears only
once.
In `@apps/web/src/pages/Telemetry/index.tsx`:
- Around line 529-542: Update the telemetry details rendering around the
readings display to memoize the raw-row string and only compute it when the
`<details>` section is open. Reuse the memoized value in the `<pre>` output
while preserving the existing empty-state message and formatting for non-empty
readings.
- Around line 474-528: Update apps/web/src/pages/Telemetry/index.tsx lines
474-528 to obtain useTranslation and route “Telemetry Data,” “Select Node,”
“Select Stat,” both select placeholders, and the raw-rows header through t(...)
using new translation keys. Update apps/web/src/components/Sidebar.tsx lines
135-139 to use t("navigation.telemetry") instead of the literal label, and add
that key to the ui namespace resources.
- Around line 264-268: Replace the tooltip construction in the chart callback
using .html() with composed DOM elements and .text() for label,
valueFmt(point.value), and dateFmt(new Date(point.time)). Preserve the existing
styling, element order, and visual appearance while ensuring decoded metric
labels are rendered as text rather than interpreted as HTML.
- Around line 44-59: Update the dimensions measurement effect in the Telemetry
page to observe containerRef.current with a ResizeObserver, invoking the
existing handleResize whenever the container changes size. Keep the initial
measurement and cleanup behavior, and disconnect the observer during effect
cleanup while retaining the window resize listener if needed.
In `@apps/web/src/pages/Telemetry/statNames.ts`:
- Around line 135-137: Update translateStatName to restrict STAT_DISPLAY_NAMES
lookups to own properties, using Object.hasOwn before returning the mapped
value; otherwise return the original name. Preserve the existing fallback
behavior for unknown metric names, including prototype keys such as constructor
and toString.
In `@apps/web/src/routes.tsx`:
- Line 7: Update the telemetry route configuration and its TelemetryPage import
so the page is loaded through the project’s lazy route mechanism (such as
lazyRouteComponent, a lazy route loader, or React.lazy) rather than eagerly
referencing the component. Preserve the existing /telemetry behavior while
deferring the d3-containing TelemetryPage module until that route is opened.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3a90460c-07aa-46cb-84a4-db43ee7296fe
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
apps/web/package.jsonapps/web/src/components/PageComponents/Telemetry/Battery.tsxapps/web/src/components/Sidebar.tsxapps/web/src/core/connections/nodeMetricsRecorder.tsapps/web/src/core/connections/sdkClient.tsapps/web/src/core/stores/deviceStore/types.tsapps/web/src/pages/Telemetry/index.tsxapps/web/src/pages/Telemetry/statNames.tsapps/web/src/routes.tsxpackage.jsonpackages/sdk-storage-sqlocal/mod.tspackages/sdk-storage-sqlocal/package.jsonpackages/sdk-storage-sqlocal/src/nodeMetrics/SqlocalNodeMetricsRepository.tspackages/sdk-storage-sqlocal/src/nodeMetrics/index.tspackages/sdk-storage-sqlocal/src/schema/index.tspackages/sdk-storage-sqlocal/src/schema/migrations.test.tspackages/sdk-storage-sqlocal/src/schema/migrations.tspackages/sdk-storage-sqlocal/src/schema/nodeMetrics.ts
| validationSchema={BluetoothValidationSchema} | ||
| defaultValues={config.bluetooth} | ||
| values={effective} | ||
| fieldGroups={[ | ||
| { | ||
| label: t("bluetooth.bluetoothConfig.label"), | ||
| description: t("bluetooth.bluetoothConfig.description"), | ||
| notes: t("bluetooth.note"), | ||
| fields: [ | ||
| { | ||
| type: "toggle", | ||
| name: "enabled", | ||
| label: t("bluetooth.enabled.label"), | ||
| description: t("bluetooth.enabled.description"), | ||
| }, | ||
| { | ||
| type: "select", | ||
| name: "mode", | ||
| label: t("bluetooth.pairingMode.label"), | ||
| description: t("bluetooth.pairingMode.description"), | ||
| properties: { | ||
| enumValue: Protobuf.Config.Config_BluetoothConfig_PairingMode, | ||
| formatEnumName: true, | ||
| }, | ||
| }, | ||
| { | ||
| type: "number", | ||
| name: "fixedPin", | ||
| label: t("bluetooth.pin.label"), | ||
| description: t("bluetooth.pin.description"), | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching Battery.tsx/config Bluetooth:"
fd -a 'Battery\.tsx|config\.proto|Bluetooth' . | sed 's#^\./##' | head -200
echo
echo "Battery outline:"
file=$(fd 'Battery\.tsx' . | head -1)
if [ -n "${file:-}" ]; then
echo "FILE=$file"
ast-grep outline "$file" --view expanded || true
echo
wc -l "$file"
sed -n '1,140p' "$file" | cat -n
fi
echo
echo "Search BluetoothValidationSchema and pairing mode usages:"
rg -n "BluetoothValidationSchema|fixedPin|pairingMode|PairingMode|NO_PIN|RANDOM_PIN|FIXED_PIN" .Repository: meshtastic/web
Length of output: 9816
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Bluetooth validation source:"
sed -n '1,80p' apps/web/src/validation/config/bluetooth.ts | cat -n
echo
echo "Bluetooth component source:"
sed -n '45,85p' apps/web/src/components/PageComponents/Telemetry/Battery.tsx | cat -n
echo
echo "Protobuf BluetoothConfig section:"
sed -n '1250,1290p' packages/protobufs/meshtastic/config.proto | cat -n
echo
echo "DynamicForm field handling:"
fd 'DynamicForm\.(ts|tsx)$' . -x sh -c 'echo "FILE=$1"; sed -n "1,220p" "$1" | cat -n' sh {}Repository: meshtastic/web
Length of output: 10314
🌐 Web query:
Zod conditional required field requiredIf exact match mode conditional optional based on another field value
💡 Result:
Zod does not have a native "requiredIf" method [1][2]. To enforce that a field is required based on the value of another field, you should use either superRefine (or .check in newer versions) for cross-field validation, or a discriminatedUnion for stronger type inference [3][4][5]. 1. Using superRefine/check (Recommended for general conditional logic) This approach allows you to inspect the entire object and add custom issues to specific fields when conditions are not met [4][6]. const schema = z.object({ conditionField: z.boolean, dependentField: z.string.optional, }).superRefine((data, ctx) => { if (data.conditionField &&!data.dependentField) { ctx.addIssue({ code: z.ZodIssueCode.custom, message: "dependentField is required when conditionField is true", path: ["dependentField"], }); } }); 2. Using Discriminated Union (Recommended for stronger types) If your conditional logic relies on literal values, a discriminated union is preferred because it creates distinct types based on the discriminator field [3][5]. const schema = z.discriminatedUnion("type", [ z.object({ type: z.literal("A"), field: z.string }), z.object({ type: z.literal("B"), field: z.string.optional }), ]); 3. Controlling when validation runs By default, refinements may only run if the base schema is valid [2][7]. If you need your custom validation to run even when base fields fail (e.g., to report all errors at once), you can use the when parameter in superRefine or .refine, which accepts a callback to determine if the refinement should execute [8][7].
Citations:
- 1: Conditional Required method for fields? colinhacks/zod#2701
- 2: What is zod's approach to conditionally requiring fields? colinhacks/zod#938
- 3: https://stackoverflow.com/questions/78083213/how-can-i-make-a-field-required-based-in-other-field-value-with-zod
- 4: Does anyone know how to conditionally validate form fields? colinhacks/zod#3561
- 5: How can I validate a field conditionally based on the value of another field in a zod form schema, ensuring all validations run simultaneously? colinhacks/zod#3268
- 6: https://zod.dev/api
- 7: [V3-V4] Superrefine is not triggered if object validation fails colinhacks/zod#5197
- 8: feat: allow 'when' parameter in .superRefine colinhacks/zod#5741
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "read-only Zod v4 behavioral probe:"
python3 - <<'PY'
from pathlib import Path
p = Path("apps/web/src/validation/config/bluetooth.ts").read_text()
print("fixedPin base requirement present:", "fixedPin: z.coerce.number().int().min(100000).max(999999)" in p)
print("fixedPin conditional refs:", any(s in p for s in ("FIXED_PIN", "fixed_pin", "superRefine", "refine", "discriminatedUnion", ".when(")))
print("mode enum values:", "'RANDOM_PIN'" in p, "'FIXED_PIN'" in p, "'NO_PIN'" in p)
PY
echo
echo "check same Bluetooth form in Settings:"
sed -n '1,90p' apps/web/src/components/PageComponents/Settings/Bluetooth.tsx | cat -nRepository: meshtastic/web
Length of output: 3656
Require fixedPin only for FIXED_PIN.
BluetoothValidationSchema requires a six-digit PIN for every Bluetooth mode, but the protobuf reserves fixedPin for FIXED_PIN; RANDOM_PIN/NO_PIN cannot be submitted without an ignored value. Make the PIN optional except when mode === FIXED_PIN, and apply the same schema UI behavior in the Bluetooth Settings form.
🤖 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 `@apps/web/src/components/PageComponents/Telemetry/Battery.tsx` around lines 51
- 81, Update BluetoothValidationSchema so fixedPin is optional for RANDOM_PIN
and NO_PIN but required as a six-digit PIN when mode is FIXED_PIN, while
preserving the existing Bluetooth validation rules. Apply the same conditional
schema and UI behavior to the Bluetooth Settings form, using the mode field and
fixedPin configuration symbols already shared by the Bluetooth forms.
| function nodeDisplayName(node: { | ||
| num: number; | ||
| user?: { longName?: string }; | ||
| }): string { | ||
| const hex = numberToHexUnpadded(node.num); | ||
| const name = | ||
| node.user?.longName || `Meshtastic ${hex.slice(-4).toUpperCase()}`; | ||
| return `${name} (!${hex})`; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# How does the rest of the app format node IDs? Compare for consistency.
rg -nP --type=ts --type=tsx -C2 'numberToHexUnpadded|padStart\(8, ?"0"\)|toString\(16\)' apps/web/src | head -80Repository: meshtastic/web
Length of output: 184
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate target file =="
fd -a 'index.tsx$' apps/web/src/pages/Telemetry 2>/dev/null || true
echo "== target snippet =="
if [ -f apps/web/src/pages/Telemetry/index.tsx ]; then
sed -n '1,80p;320,360p' apps/web/src/pages/Telemetry/index.tsx | cat -n
fi
echo "== search node ID formatting in apps/web/src =="
rg -n -C2 'numberToHexUnpadded|padStart\(8, ?"0"\)|toString\(16\)|!0[a-fA-F0-9]{8}|num' apps/web/src --glob '*.ts' --glob '*.tsx' | head -120Repository: meshtastic/web
Length of output: 12499
🌐 Web query:
Meshtastic NodeID hex ID 8 digits zero pad example
💡 Result:
In Meshtastic, the "Node ID" displayed in many client applications is a user-friendly string representation of the underlying 32-bit Node Number (NodeNum) [1][2]. This string format consists of an exclamation mark (!) followed by 8 hexadecimal characters, which are zero-padded to ensure a fixed length [1][3][4]. Example format:!xxxxxxxx For instance, if a node has a NodeNum of 0x12345 (decimal 74565), its short-form Node ID would be:!00012345 Technical implementation notes: - Generation: In the firmware, this is typically generated using a format string such as "!%08x" [3][5]. - Purpose: This 8-character hex string is a convenience for UI display and is not used on the mesh wire protocol itself, which relies on the raw 32-bit integer [1]. - Standardization: Recent updates to the firmware have centralized this formatting logic into a method often called getNodeId to ensure consistency across different components of the system [6].
Citations:
- 1: https://github.com/meshtastic/meshtastic-sdk/blob/main/docs/glossary.md
- 2: [Bug]: inconsistent use of Node Id vs User Id in UI Meshtastic-Android#2408
- 3: https://github.com/meshtastic/firmware/blob/fb6d199d/src/mesh/NodeDB.cpp
- 4: https://python.meshtastic.org/mesh_interface.html
- 5: https://github.com/meshtastic/firmware/blob/master/src/modules/NodeInfoModule.cpp
- 6: Centralize getNodeId and fix references to owner.id firmware#8230
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
def unpadded(n):
return format(n & 0xFFFFFFFF, "x") or "0"
def padded(n):
return format(n & 0xFFFFFFFF, "08x")
cases = [
12345,
0x00012345,
0x05abc123,
0xffffffff,
]
for n in cases:
h = unpadded(n)
print(f"{n:010d} 0x{n:08x}: unpadded={h!r} padded={padded(n)} fallback_short={h[-4:].upper()} padded_fallback={padded(n)[-4:].upper()}")
PYRepository: meshtastic/web
Length of output: 544
Format node IDs with 8 lowercase hex digits.
numberToHexUnpadded omits leading zeros, so IDs below 0x10000000 render non-canonical strings like !5abc123; Meshtastic node ID display uses !xxxxxxxx. Use a fixed 8-char hex value for both the displayed ID and the !XXXX suffix.
🤖 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 `@apps/web/src/pages/Telemetry/index.tsx` around lines 342 - 350, Update
nodeDisplayName to format node.num as exactly 8 lowercase hexadecimal
characters, padding leading zeros as needed; use this canonical value for both
the fallback name’s hex suffix and the returned !node ID while preserving the
existing uppercase presentation of the fallback suffix.
| if (policy.maxPerMetric !== undefined) { | ||
| const max = policy.maxPerMetric; | ||
| // Trim each over-cap (node, metric) bucket down to the newest `max` rows. | ||
| const overCap = await this.db | ||
| .select({ | ||
| nodeNum: nodeMetrics.nodeNum, | ||
| metric: nodeMetrics.metric, | ||
| c: count(), | ||
| }) | ||
| .from(nodeMetrics) | ||
| .where(eq(nodeMetrics.deviceId, this.deviceId)) | ||
| .groupBy(nodeMetrics.nodeNum, nodeMetrics.metric) | ||
| .having(sql`count(*) > ${max}`); | ||
| for (const row of overCap) { | ||
| const cutoffRows = await this.db | ||
| .select({ ts: nodeMetrics.ts }) | ||
| .from(nodeMetrics) | ||
| .where( | ||
| and( | ||
| eq(nodeMetrics.deviceId, this.deviceId), | ||
| eq(nodeMetrics.nodeNum, row.nodeNum), | ||
| eq(nodeMetrics.metric, row.metric), | ||
| )!, | ||
| ) | ||
| .orderBy(desc(nodeMetrics.ts)) | ||
| .limit(1) | ||
| .offset(max - 1); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg 'SqlocalNodeMetricsRepository\.ts$' || true
echo "== file excerpt =="
file="$(git ls-files | rg 'packages/sdk-storage-sqlocal/src/nodeMetrics/SqlocalNodeMetricsRepository\.ts$' | head -n1)"
if [ -n "${file:-}" ]; then
wc -l "$file"
sed -n '1,180p' "$file" | cat -n
fi
echo "== search maxPerMetric usages =="
rg -n "maxPerMetric|nodeMetrics" packages/sdk-storage-sqlocal/src packages -g '*.ts' | head -n 200Repository: meshtastic/web
Length of output: 16783
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== nodeMetrics schema =="
sed -n '1,60p' packages/sdk-storage-sqlocal/src/schema/nodeMetrics.ts.ts 2>/dev/null || sed -n '1,60p' packages/sdk-storage-sqlocal/src/schema/nodeMetrics.ts | cat -n
echo "== Sqlocal DB definition/Driver usages =="
rg -n "class SqlocalDb|type SqlocalDb|SqlocalDb|Drizzle|offset\\(|limit\\(" packages/sdk-storage-sqlocal/src -g '*.ts' | head -n 200
echo "== drizzle package versions if present =="
(fd -a 'package.json$' . | while read -r f; do
echo "--- $f"
jq -r '.dependencies.drizzle-orm // .devDependencies.drizzle-orm // "none"' "$f" 2>/dev/null || true
done)
echo "== static JS/SQL semantics probe for pruned offset expression =="
node - <<'JS'
const bad = [undefined, null, -1, 0, 1, 1.5, Infinity, 2e308, "0", 0n];
for (const raw of bad) {
let max;
try {
max = raw == null ? undefined : Number(raw);
} catch (e) {
max = "ERR";
}
let rowsToKeep = undefined;
let deleteCutoffTimestamp = undefined;
if (max !== undefined) {
const overCapCondition = Number(42) > max;
if (overCapCondition) {
rowsToKeep = max;
deleteCutoffTimestamp = max - 1;
}
}
console.log(JSON.stringify({ raw, parsed: max, overCapWith42: rowsToKeep !== undefined, deleteIfAboveFourtytwo: rowsToKeep, deleteCutoffTimestamp }));
}
JSRepository: meshtastic/web
Length of output: 9073
🌐 Web query:
Drizzle sql offset invalid value SQLite behavior offset parameter
💡 Result:
In SQLite, using an OFFSET clause without a preceding LIMIT clause is a syntax error [1][2][3]. The SQLite language specification requires that if an OFFSET clause is present, it must follow a LIMIT clause [4][5]. When using Drizzle ORM with SQLite, this requirement means you cannot simply omit the LIMIT parameter if you wish to use an OFFSET [2][6]. Attempting to generate a query with only an OFFSET in SQLite will cause the database driver to throw a syntax error (e.g., "near OFFSET: syntax error") [2][3][7]. To use an OFFSET in SQLite without actually limiting the number of results, the standard workaround is to provide a negative value for the LIMIT clause [1][2]. In SQLite, a negative LIMIT indicates that there is no upper bound on the number of rows returned, effectively allowing the query to function as an "offset-only" operation [1][6][4]. For Drizzle ORM users, if you need this behavior, you can implement it by explicitly setting the limit to -1: // Example of offset without functional limit in SQLite await db.select.from(users).orderBy(users.id).limit(-1) // Required for valid SQLite syntax when using offset.offset(50); Some versions or configurations of Drizzle ORM have included fixes to automatically handle this requirement [1][6], but explicitly including.limit(-1) remains a reliable way to ensure the generated SQL remains valid across SQLite environments [2].
Citations:
- 1: fix(drizzle-orm): fix usage of offset without limit in SQLite dialect drizzle-team/drizzle-orm#4561
- 2: [BUG]: Negative limit not working in SQLite drizzle-team/drizzle-orm#2992
- 3: Attempt to build a request with OFFSET but no LIMIT causes SQL error go-gorm/sqlite#53
- 4: https://www.sqlite.org/lang_select.html
- 5: https://sqlite.org/syntax/select-stmt.html
- 6: https://github.com/drizzle-team/drizzle-orm/pull/4561/commits
- 7: SQLite cannot use OFFSET MasoniteFramework/orm#929
🌐 Web query:
SQLocalDrizzle sqlocal offset limit SQLite
💡 Result:
When using Drizzle ORM with SQLocal (which serves as a driver for SQLite), you apply limit and offset pagination using the standard Drizzle ORM APIs [1][2][3]. Because SQLocal facilitates standard SQLite execution, you must adhere to SQLite's specific requirements regarding these clauses [4][5][6]. Core Considerations for SQLite Pagination: 1. Mandatory LIMIT with OFFSET: In SQLite, you cannot use an OFFSET clause without also including a LIMIT clause [4][5]. Attempting a query with only OFFSET will result in a syntax error [5]. 2. Handling OFFSET without a capped result set: If you need to skip a number of rows but want all subsequent rows (effectively no limit), you must provide a negative value for the LIMIT clause [5]. The standard practice in SQLite is to use LIMIT -1 [4][5]. Example Implementation: If you are using Drizzle's query builder (select syntax): await db.select.from(users).limit(10) // Number of rows to return [7].offset(20); // Number of rows to skip [8][7] For scenarios where you only need an offset (no upper bound), use -1 for the limit: await db.select.from(users).limit(-1) // Required for valid SQLite syntax when using offset [4][5].offset(50); If you are using Drizzle's relational query API (findMany): await db.query.users.findMany({ limit: 10, offset: 20, }); If you encounter issues where Drizzle's abstraction prevents a negative limit, you can use a placeholder to bypass validation [5]: await db.query.users.findMany({ offset: 20, limit: sql.placeholder('limit'), }).prepare.execute({ limit: -1 }); [5]
Citations:
- 1: https://sqlocal.dev/drizzle/setup
- 2: https://github.com/DallasHoff/sqlocal
- 3: https://sqlocal.dev/guide/introduction
- 4: fix(drizzle-orm): fix usage of offset without limit in SQLite dialect drizzle-team/drizzle-orm#4561
- 5: [BUG]: Negative limit not working in SQLite drizzle-team/drizzle-orm#2992
- 6: https://sqlocal.dev/
- 7: https://orm.drizzle.team/docs/guides/limit-offset-pagination
- 8: https://drizzle-team-drizzle-orm.mintlify.app/api/sqlite/query-builders
Validate maxPerMetric before deriving the offset.
maxPerMetric can currently be any number, so values like 0, -1, 1.5, or infinity can produce incorrect/invalid retention SQL. Reject invalid values before pruning, or explicitly document that 0 clears each bucket.
Proposed guard
if (policy.maxPerMetric !== undefined) {
const max = policy.maxPerMetric;
+ if (!Number.isSafeInteger(max) || max < 1) {
+ throw new RangeError("maxPerMetric must be a positive integer");
+ }
// Trim each over-cap (node, metric) bucket down to the newest `max` rows.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (policy.maxPerMetric !== undefined) { | |
| const max = policy.maxPerMetric; | |
| // Trim each over-cap (node, metric) bucket down to the newest `max` rows. | |
| const overCap = await this.db | |
| .select({ | |
| nodeNum: nodeMetrics.nodeNum, | |
| metric: nodeMetrics.metric, | |
| c: count(), | |
| }) | |
| .from(nodeMetrics) | |
| .where(eq(nodeMetrics.deviceId, this.deviceId)) | |
| .groupBy(nodeMetrics.nodeNum, nodeMetrics.metric) | |
| .having(sql`count(*) > ${max}`); | |
| for (const row of overCap) { | |
| const cutoffRows = await this.db | |
| .select({ ts: nodeMetrics.ts }) | |
| .from(nodeMetrics) | |
| .where( | |
| and( | |
| eq(nodeMetrics.deviceId, this.deviceId), | |
| eq(nodeMetrics.nodeNum, row.nodeNum), | |
| eq(nodeMetrics.metric, row.metric), | |
| )!, | |
| ) | |
| .orderBy(desc(nodeMetrics.ts)) | |
| .limit(1) | |
| .offset(max - 1); | |
| if (policy.maxPerMetric !== undefined) { | |
| const max = policy.maxPerMetric; | |
| if (!Number.isSafeInteger(max) || max < 1) { | |
| throw new RangeError("maxPerMetric must be a positive integer"); | |
| } | |
| // Trim each over-cap (node, metric) bucket down to the newest `max` rows. | |
| const overCap = await this.db | |
| .select({ | |
| nodeNum: nodeMetrics.nodeNum, | |
| metric: nodeMetrics.metric, | |
| c: count(), | |
| }) | |
| .from(nodeMetrics) | |
| .where(eq(nodeMetrics.deviceId, this.deviceId)) | |
| .groupBy(nodeMetrics.nodeNum, nodeMetrics.metric) | |
| .having(sql`count(*) > ${max}`); | |
| for (const row of overCap) { | |
| const cutoffRows = await this.db | |
| .select({ ts: nodeMetrics.ts }) | |
| .from(nodeMetrics) | |
| .where( | |
| and( | |
| eq(nodeMetrics.deviceId, this.deviceId), | |
| eq(nodeMetrics.nodeNum, row.nodeNum), | |
| eq(nodeMetrics.metric, row.metric), | |
| )!, | |
| ) | |
| .orderBy(desc(nodeMetrics.ts)) | |
| .limit(1) | |
| .offset(max - 1); |
🤖 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 `@packages/sdk-storage-sqlocal/src/nodeMetrics/SqlocalNodeMetricsRepository.ts`
around lines 102 - 128, Validate policy.maxPerMetric before using it to derive
the query offset in the retention logic: accept only a finite, non-negative
integer, or explicitly implement and preserve the documented behavior that zero
clears each bucket. Reject invalid values such as negatives, fractions, and
Infinity before the overCap query and pruning loop run.
1 participant
Description
Adds Telemetry UI (#1244)
Related Issues
Changes Made
Testing Done
Screenshots (if applicable)
Checklist
CONTRIBUTING_I18N_DEVELOPER_GUIDE.md for more details)
Summary by CodeRabbit