add protobuf #167
add protobuf #167
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
DNM - DO NOT MERGE!!! |
ideen1
force-pushed
the
main
branch
3 times, most recently
from
e6a0207 to
a41ac25
Compare
burtonjong
force-pushed
the
main
branch
3 times, most recently
from
49e1bfd to
2622e7a
Compare
justin-phxm
force-pushed
the
add_protobuf
branch
from
1d8eadd to
d781ec6
Compare
justin-phxm
force-pushed
the
add_protobuf
branch
from
411dfd6 to
f47a5a3
Compare
justin-phxm
force-pushed
the
add_protobuf
branch
from
f47a5a3 to
01b8a38
Compare
justin-phxm
force-pushed
the
add_protobuf
branch
from
01b8a38 to
31310e2
Compare
justin-phxm
force-pushed
the
add_protobuf
branch
from
31310e2 to
f890f2d
Compare
justin-phxm
force-pushed
the
add_protobuf
branch
from
f890f2d to
4b61ad2
Compare
WalkthroughMigrates telemetry from io-ts runtime validation to protobuf-based types: removes io-ts codecs and validators, adds proto schema and generation script, adjusts generated/consuming code (field renames, deleted fake data), updates MQTT handling to cast to ITelemetryData, and adds test infra and docs updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Broker as MQTT Broker
participant Server as SolarMQTTClient (Server)
participant Types as Shared (proto Types)
participant Handler as Application Handler / DB
Broker->>Server: MQTT message (packet bytes)
Server->>Server: JSON.parse(packet)
Server->>Types: cast parsed object to ITelemetryData
Note right of Types: No runtime validation (cast only)
Server->>Handler: handlePacketReceive(ITelemetryData)
Handler->>Handler: process and persist (DB / downstream)
Handler-->>Server: ack/response (if any)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🪛 Buf (1.61.0)packages/shared/src/protoTypes.proto3-3: Files with package "telemetry" must be within a directory "telemetry" relative to root but were in directory "packages/shared/src". (PACKAGE_DIRECTORY_MATCH) 🔇 Additional comments (1)
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/server/src/datasources/SolarMQTTClient/SolarMQTTClient.ts (1)
103-111: MoveJSON.parseinside the try/catch and guardpacket.Pi?.Rfid(current code can crash outside the catch).
Right now a malformed MQTT payload throws before thetry, andpacket.Pi.Rfid.toString()can also throw. Also fix Biome’snoSwitchDeclarationsby block-scoping the case.case packetTopic: - const packet = JSON.parse(message.toString()) as ITelemetryData; - try { - this.backendController.handlePacketReceive(packet); - this.latestRfid = packet.Pi.Rfid.toString(); - } catch (error) { - logger.error(`Invalid packet format: ${error.message}`); - } + { + try { + const packet = JSON.parse(message.toString()) as ITelemetryData; + this.backendController.handlePacketReceive(packet); + this.latestRfid = packet.Pi?.Rfid ? String(packet.Pi.Rfid) : null; + } catch (error) { + logger.error(`Invalid packet format: ${error.message}`); + } + } break;
🧹 Nitpick comments (4)
packages/client/src/components/containers/Plotly.tsx (1)
36-36: Consider adding a data-testid or similar attribute for testing.With the removal of the
idattribute, tests that previously relied on DOM selection via ID may need an alternative. Consider accepting an optionaldata-testidprop for test identification.- ({ className, config, data, layout }, ref) => { + ({ className, config, data, layout, "data-testid": dataTestId }, ref) => { const originRef = useRef(null); const [handle, setHandle] = useState<Plotly.PlotlyHTMLElement>(); useEffect(() => { let instance: Plotly.PlotlyHTMLElement | undefined; if (originRef.current) { newPlot(originRef.current, data, layout, config).then((ref) => { setHandle((instance = ref)); }); } return () => { if (instance) { purge(instance); } }; }, [data, layout, config]); useImperativeHandle(ref, () => handle as PlotlyHTML, [handle]); - return <div className={className} ref={originRef} />; + return <div className={className} ref={originRef} data-testid={dataTestId} />; },packages/shared/.eslintrc.cjs (1)
5-7: Avoid disablingno-consolefor the whole workspace; use targetedoverridesinstead.
Right now this enables console usage everywhere inpackages/shared. Prefer scoping it to scripts (e.g.,src/test.ts) or specific directories so accidental debug logs don’t slip into library code.packages/shared/src/.proto (1)
7-351: Consider adopting snake_case for field names per Protobuf style conventions.Protobuf style guides recommend snake_case for field names to avoid naming collisions when identifiers are converted to different language styles. Your schema uses PascalCase throughout (e.g.,
CclReducedDueToAlternateCurrentLimit,AlwaysOnSignalStatus), which deviates from standard Protobuf conventions. If this is intentional to preserve compatibility with the legacy io-ts types, document the decision; otherwise, consider migrating to snake_case field names.packages/shared/src/types.ts (1)
28-35: Simplify the RemoveUndefined type application.The double application of
RemoveUndefined<RemoveUndefined<TelemetryData>>on line 35 is redundant. The first application removes all undefined from the type; the second pass has no effect. Simplify to:ITelemetryData = RemoveUndefined<TelemetryData>;- export type ITelemetryData = RemoveUndefined<RemoveUndefined<TelemetryData>>; + export type ITelemetryData = RemoveUndefined<TelemetryData>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
README.md(4 hunks)packages/client/src/components/containers/Plotly.tsx(2 hunks)packages/client/src/components/molecules/PlaybackMolecules/fakedata.ts(0 hunks)packages/client/src/hooks/PIS/PIS.motor.tsx(4 hunks)packages/server/src/datasources/SolarMQTTClient/SolarMQTTClient.ts(2 hunks)packages/shared/.eslintrc.cjs(1 hunks)packages/shared/package.json(1 hunks)packages/shared/src/.proto(1 hunks)packages/shared/src/README.md(1 hunks)packages/shared/src/functions.ts(4 hunks)packages/shared/src/test.ts(1 hunks)packages/shared/src/types.ts(2 hunks)
💤 Files with no reviewable changes (1)
- packages/client/src/components/molecules/PlaybackMolecules/fakedata.ts
🧰 Additional context used
🧬 Code graph analysis (3)
packages/server/src/datasources/SolarMQTTClient/SolarMQTTClient.ts (1)
packages/shared/src/types.ts (1)
ITelemetryData(35-35)
packages/shared/src/test.ts (2)
packages/shared/src/functions.ts (1)
generateFakeTelemetryData(9-412)packages/shared/src/protoTypes.ts (2)
B3(95-118)B3(1999-2438)
packages/shared/src/types.ts (1)
packages/shared/src/protoTypes.ts (2)
TelemetryData(338-353)TelemetryData(6513-6855)
🪛 Biome (2.1.2)
packages/server/src/datasources/SolarMQTTClient/SolarMQTTClient.ts
[error] 104-104: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🪛 Buf (1.61.0)
packages/shared/src/.proto
3-3: Files with package "telemetry" must be within a directory "telemetry" relative to root but were in directory "packages/shared/src".
(PACKAGE_DIRECTORY_MATCH)
🪛 markdownlint-cli2 (0.18.1)
README.md
72-72: Link fragments should be valid
(MD051, link-fragments)
73-73: Link fragments should be valid
(MD051, link-fragments)
74-74: Link fragments should be valid
(MD051, link-fragments)
75-75: Link fragments should be valid
(MD051, link-fragments)
76-76: Link fragments should be valid
(MD051, link-fragments)
🔇 Additional comments (5)
packages/client/src/components/containers/Plotly.tsx (2)
9-9: Verify thatPlotParamsis exported from react-plotly.js.Ensure that the
PlotParamstype is available in the version of react-plotly.js used by this project, as the entire component depends on this import.Run the following script to check the export:
#!/bin/bash # Description: Verify PlotParams is exported from react-plotly.js # Check package.json for react-plotly.js version fd -t f "package.json" --exec jq -r '.dependencies["react-plotly.js"] // .devDependencies["react-plotly.js"] // empty' {} \; # Search for PlotParams usage in node_modules type definitions fd -t f -e d.ts . node_modules/@types/react-plotly.js --exec rg -n "PlotParams" {} \; 2>/dev/null || echo "Type definitions not found in node_modules"Additionally, search the web to confirm PlotParams is part of the react-plotly.js API:
react-plotly.js PlotParams type definition
15-16: Theidprop removal does not break any existing code. The only usage of the Plotly component in the codebase (MLContainer.tsx, line 71) does not pass anidprop—it only usesclassName,config,data, andlayout.packages/client/src/hooks/PIS/PIS.motor.tsx (1)
48-66: Field rename usage looks consistent (bemfD/bemfQ/DCBusAh)—please verify exact casing against generated types.
This is a clean mechanical rename; just ensure the upstream payload (protobuf/JSON) actually producesDCBusAh(not e.g.DC_Bus_Ah).Also applies to: 224-242
packages/shared/src/functions.ts (1)
2-2: Fake telemetry generator updates for motor fields look consistent.
The rename toDCBusAh/bemfD/bemfQaligns with the client updates; no issues in this segment.Also applies to: 300-361
README.md (1)
120-125: [rewritten review comment]
[classification tag]
| }; | ||
| }, [data, layout, config]); | ||
|
|
||
| useImperativeHandle(ref, () => handle as PlotlyHTML, [handle]); |
There was a problem hiding this comment.
Unsafe cast: handle can be undefined.
The cast handle as PlotlyHTML is unsafe because handle can be undefined (especially before the useEffect runs). This can cause runtime errors for consumers who access the ref before the Plotly instance is initialized.
After fixing the state typing at line 18, update the cast to handle the undefined case:
- useImperativeHandle(ref, () => handle as PlotlyHTML, [handle]);
+ useImperativeHandle(ref, () => handle as PlotlyHTML | undefined, [handle]);Or use a type guard:
- useImperativeHandle(ref, () => handle as PlotlyHTML, [handle]);
+ useImperativeHandle(
+ ref,
+ () => (handle ? (handle as PlotlyHTML) : undefined),
+ [handle]
+ );📝 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.
| useImperativeHandle(ref, () => handle as PlotlyHTML, [handle]); | |
| useImperativeHandle(ref, () => handle as PlotlyHTML | undefined, [handle]); |
🤖 Prompt for AI Agents
In packages/client/src/components/containers/Plotly.tsx around line 34, the cast
useImperativeHandle(ref, () => handle as PlotlyHTML, [handle]) is unsafe because
handle can be undefined; update the code to explicitly account for the undefined
case (e.g., return handle as PlotlyHTML | undefined or null, or return a guarded
object only when handle is defined) and adjust the forwardRef/generic and the
state typing declared at line 18 so the ref type reflects that the Plotly
instance may be absent until initialized; ensure consumers' ref type accepts
undefined/null accordingly.
| "dependencies": { | ||
| "@aws-sdk/client-secrets-manager": "^3.658.1", | ||
| "fp-ts": "^2.16.9", | ||
| "io-ts": "^2.2.22", | ||
| "typescript": "^5.5.3" | ||
| "@bufbuild/protobuf": "^2.6.1", | ||
| "protobufjs": "^7.4.0" | ||
| }, | ||
| "devDependencies": { | ||
| "@faker-js/faker": "^9.0.3" | ||
| "@faker-js/faker": "^9.0.3", | ||
| "ts-proto": "^2.2.5", | ||
| "typescript": "^5.6.3" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
ts-proto v2.2.5 generated code default runtime protobufjs @bufbuild/protobuf
💡 Result:
Short answer: ts-proto v2.x (including v2.2.5) uses @bufbuild/protobuf as the default low‑level runtime (moving away from protobufjs). That means generated encode/decode now depend on @bufbuild/protobuf (e.g. BinaryReader/BinaryWriter from "@bufbuild/protobuf/wire"); if you relied on protobufjs internals you must update code or pin ts‑proto 1.x. [1][2][3]
Useful links:
- ts-proto README (notes about 2.x migration to @bufbuild/protobuf). [1]
- @bufbuild/protobuf package / docs. [2]
- npm/summary mentioning the migration and migration advice to pin 1.x if needed. [3]
If you want, I can:
- show concrete code diffs for migrating Reader/Writer usage to @bufbuild/protobuf, or
- show how to pin ts-proto to 1.x in package.json.
🏁 Script executed:
# Search for protoTypes.ts and related generated protobuf files
find . -name "*proto*" -type f | grep -E "\.(ts|js)$" | head -20Repository: UCSolarCarTeam/Helios-Telemetry
Length of output: 110
🏁 Script executed:
# Check if either runtime is imported in the codebase
rg "from ['\"]@bufbuild/protobuf['\"]|from ['\"]protobufjs['\"]|import.*protobufjs|import.*@bufbuild" --type ts --type jsRepository: UCSolarCarTeam/Helios-Telemetry
Length of output: 178
🏁 Script executed:
# Check the actual dependencies in package.json to understand context
cat packages/shared/package.jsonRepository: UCSolarCarTeam/Helios-Telemetry
Length of output: 435
Remove unused protobufjs dependency.
ts-proto v2.2.5 generates code that depends exclusively on @bufbuild/protobuf (notably BinaryReader and BinaryWriter from @bufbuild/protobuf/wire). The codebase in packages/shared/src/protoTypes.ts confirms this—protobufjs is not imported or used. Drop it to reduce the dependency surface area.
🤖 Prompt for AI Agents
In packages/shared/package.json around lines 6 to 15, remove the unused
"protobufjs" dependency from the "dependencies" object because ts-proto output
and our code use @bufbuild/protobuf only; update package.json to delete the
"protobufjs" entry and then run the package manager (npm/yarn/pnpm) to update
lockfile and node_modules (install or prune) so the dependency is fully removed.
| # Instructions to test | ||
|
|
||
| Ensure you have the protobuf server running on port 8000. You can find additional instructions in the [protobuf server README](https://github.com/UCSolarCarTeam/ML-Telemetry/blob/setup_protobuf/README.md#generate-python-code-from-protobuf) | ||
|
|
||
| To run the tests for the shared package, you can use the following command in the root directory of the repository: | ||
|
|
||
| ```bash | ||
| npx tsx ./packages/shared/src/test.ts | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Avoid linking to a moving branch for setup docs; pin to a commit or mirror essentials here.
Branch links tend to break; consider pointing at a stable branch/tag/commit (or duplicating the minimal “run server on :8000” steps).
🤖 Prompt for AI Agents
In packages/shared/src/README.md around lines 1 to 10, the README currently
links to a moving branch for protobuf server setup which can break; update the
link to a specific stable tag/commit (or a release) or replace the external link
by copying the minimal required setup steps (how to generate/run the protobuf
server on port 8000) into this README so tests remain reproducible; ensure the
link, if kept, points to a commit hash or tag and add the essential port/command
instructions inline.
| import axios from "axios"; | ||
| import { generateFakeTelemetryData } from "./functions"; | ||
| import { B3 as B3Proto } from "./protoTypes"; | ||
| const { B3 } = generateFakeTelemetryData(); | ||
| async function sendData() { | ||
| try { | ||
| const payload = B3Proto.create(B3); | ||
| const bytes = B3Proto.encode(payload).finish(); | ||
| const { data } = await axios.post("http://localhost:8000/receive", bytes, { | ||
| headers: { "Content-Type": "application/octet-stream" }, | ||
| responseType: "arraybuffer", | ||
| }); | ||
|
|
||
| const decodedObject = B3Proto.decode(data); | ||
| console.log(decodedObject); | ||
| } catch (error) { | ||
| console.error("Error occurred:", error); | ||
| } | ||
| } | ||
|
|
||
| sendData(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
axios responseType arraybuffer return type
💡 Result:
Short answer: when you set axios config responseType: 'arraybuffer', axios returns binary data in response.data as an ArrayBuffer (TypeScript: AxiosResponse). In practice in Node you can treat/write it as a Buffer-like binary block and in the browser it is an ArrayBuffer (use Uint8Array, Blob, etc.). Handle errors specially because non-2xx error bodies will also be delivered as an ArrayBuffer and must be decoded to text/JSON if needed. [1][2][3]
Sources:
- MDN: XMLHttpRequest responseType 'arraybuffer' → response is an ArrayBuffer. [1]
- Axios docs / types: response.data is parsed according to responseType; TS generics let AxiosResponse (T = ArrayBuffer for responseType 'arraybuffer'). [2]
- Discussion/examples: error responses with responseType 'arraybuffer' arrive as ArrayBuffer and must be decoded (e.g. TextDecoder/JSON.parse). [3]
🌐 Web query:
protobufjs decode method accepts ArrayBuffer or Uint8Array
💡 Result:
Protobuf.js Message.decode expects a Reader or a Uint8Array — it does not accept an ArrayBuffer directly. Pass a Uint8Array (or Buffer in Node) — if you have an ArrayBuffer, wrap it: new Uint8Array(myArrayBuffer). [1][2]
Example:
const msg = MyMessage.decode(new Uint8Array(arrayBuffer));
Sources: Protobuf.js API docs (Message.decode signature) and Reader doc. [1] [2]
Fix decode input type and add a request timeout (prevent runtime errors and hanging scripts).
With responseType: "arraybuffer", axios returns data as an ArrayBuffer, but B3Proto.decode() requires a Uint8Array. Additionally, add a timeout to prevent the script from hanging indefinitely if the server is unavailable.
- const { data } = await axios.post("http://localhost:8000/receive", bytes, {
+ const { data } = await axios.post("http://localhost:8000/receive", bytes, {
headers: { "Content-Type": "application/octet-stream" },
responseType: "arraybuffer",
+ timeout: 10_000,
});
- const decodedObject = B3Proto.decode(data);
+ const decodedObject = B3Proto.decode(new Uint8Array(data));
console.log(decodedObject);📝 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.
| import axios from "axios"; | |
| import { generateFakeTelemetryData } from "./functions"; | |
| import { B3 as B3Proto } from "./protoTypes"; | |
| const { B3 } = generateFakeTelemetryData(); | |
| async function sendData() { | |
| try { | |
| const payload = B3Proto.create(B3); | |
| const bytes = B3Proto.encode(payload).finish(); | |
| const { data } = await axios.post("http://localhost:8000/receive", bytes, { | |
| headers: { "Content-Type": "application/octet-stream" }, | |
| responseType: "arraybuffer", | |
| }); | |
| const decodedObject = B3Proto.decode(data); | |
| console.log(decodedObject); | |
| } catch (error) { | |
| console.error("Error occurred:", error); | |
| } | |
| } | |
| sendData(); | |
| import axios from "axios"; | |
| import { generateFakeTelemetryData } from "./functions"; | |
| import { B3 as B3Proto } from "./protoTypes"; | |
| const { B3 } = generateFakeTelemetryData(); | |
| async function sendData() { | |
| try { | |
| const payload = B3Proto.create(B3); | |
| const bytes = B3Proto.encode(payload).finish(); | |
| const { data } = await axios.post("http://localhost:8000/receive", bytes, { | |
| headers: { "Content-Type": "application/octet-stream" }, | |
| responseType: "arraybuffer", | |
| timeout: 10_000, | |
| }); | |
| const decodedObject = B3Proto.decode(new Uint8Array(data)); | |
| console.log(decodedObject); | |
| } catch (error) { | |
| console.error("Error occurred:", error); | |
| } | |
| } | |
| sendData(); |
🤖 Prompt for AI Agents
In packages/shared/src/test.ts around lines 1 to 21, the code passes axios
response.data (an ArrayBuffer) directly to B3Proto.decode which expects a
Uint8Array and also lacks a request timeout; convert the response ArrayBuffer to
a Uint8Array before decoding (e.g., new Uint8Array(data)) and add a timeout
option to the axios.post call (for example timeout: 5000) so the script won’t
hang if the server is unreachable; also ensure the request body you send is the
bytes (Uint8Array) or ArrayBuffer as appropriate so the server receives binary
data correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/shared/src/protoTypes.proto (1)
3-4: Buf lint will fail: package↔directory mismatch (PACKAGE_DIRECTORY_MATCH).
This still declarespackage telemetry;while living atpackages/shared/src/protoTypes.proto, which violates Buf’s PACKAGE_DIRECTORY_MATCH in the provided static hint.Proposed direction (pick one and update imports/tooling accordingly):
-// packages/shared/src/protoTypes.proto +// packages/shared/src/telemetry/protoTypes.proto syntax = "proto3"; package telemetry;or version it:
-// packages/shared/src/protoTypes.proto +// packages/shared/src/telemetry/v1/telemetry.proto syntax = "proto3"; -package telemetry; +package telemetry.v1;
🧹 Nitpick comments (2)
packages/shared/src/protoTypes.proto (2)
7-352: Field names are non-idiomatic for protobuf; verify lint + ts-proto output expectations.
Most fields are PascalCase (and some embed numeric suffixes likeMotorDetails0), whereas protobuf convention (and many linters) expectlower_snake_case. If ts-proto consumers rely on current casing, codify that in generation options; otherwise consider renaming now before it becomes a compatibility burden.Example shape:
message TelemetryData { - MotorDetails MotorDetails0 = 8; - MotorDetails MotorDetails1 = 9; - uint64 TimeStamp = 13; - string Title = 14; + MotorDetails motor_details_0 = 8; + MotorDetails motor_details_1 = 9; + uint64 timestamp = 13; + string title = 14; }
7-352: Proto3 presence semantics: confirm whether “unset vs zero/false” matters for telemetry frames.
If frames can be partial (missing sensors, dropped CAN packets, etc.), proto3 scalars won’t let you distinguish “missing” from “0/false”. Consideroptionalon fields where presence matters, or use wrapper types for scalars.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
package.json(1 hunks)packages/shared/src/protoTypes.proto(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🧰 Additional context used
🪛 Buf (1.61.0)
packages/shared/src/protoTypes.proto
3-3: Files with package "telemetry" must be within a directory "telemetry" relative to root but were in directory "packages/shared/src".
(PACKAGE_DIRECTORY_MATCH)
| message TelemetryData { | ||
| B3 B3 = 1; | ||
| BatteryStatus Battery = 2; | ||
| BatteryFaults BatteryFaults = 3; | ||
| ContactorStatus Contactor = 4; | ||
| KeyMotor KeyMotor = 5; | ||
| MBMS MBMS = 6; | ||
| MPPT MPPT = 7; | ||
| MotorDetails MotorDetails0 = 8; | ||
| MotorDetails MotorDetails1 = 9; | ||
| Pi Pi = 10; | ||
| ProximitySensors ProximitySensors = 11; | ||
| Telemetry Telemetry = 12; | ||
| uint64 TimeStamp = 13; | ||
| string Title = 14; | ||
| } |
There was a problem hiding this comment.
TimeStamp needs a defined unit/epoch (and consider google.protobuf.Timestamp).
As-is, uint64 TimeStamp = 13; is ambiguous (ms? µs? ns? unix seconds?), which tends to cause subtle client/server bugs. Either add an explicit comment + enforce the unit everywhere, or switch to google.protobuf.Timestamp.
Possible change:
syntax = "proto3";
+import "google/protobuf/timestamp.proto";
message TelemetryData {
...
- uint64 TimeStamp = 13;
+ google.protobuf.Timestamp timestamp = 13;
string Title = 14;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/shared/src/protoTypes.proto around lines 337 to 352, the TimeStamp
field is ambiguous (uint64 TimeStamp = 13) and must have a defined unit/epoch or
be converted to google.protobuf.Timestamp; either (A) change the field to
google.protobuf.Timestamp TimeStamp = 13, add import
"google/protobuf/timestamp.proto", update all producers/consumers to use
Timestamp values and regenerate protobuf artifacts, or (B) keep uint64 but add a
clear comment specifying the epoch and unit (e.g., "uint64 TimeStamp = 13; //
unix epoch milliseconds"), update all codepaths to consistently read/write that
unit, add validation/tests to enforce it, and regenerate artifacts — pick one
approach and apply it project-wide.
cf5cfc7 to
804bc98
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/shared/src/protoTypes.proto (2)
1-4: Buf lint/package-directory mismatch will likely fail CI (packagetelemetryvs path).
Withpackage telemetry;, Buf’sPACKAGE_DIRECTORY_MATCHexpects the file to live under atelemetry/directory relative to the module root; currently it’spackages/shared/src/protoTypes.proto.Action: either move/rename to something like
packages/shared/src/telemetry/protoTypes.proto(and update generation paths), or change the declaredpackageto match the directory structure you want.
337-352:TimeStampis ambiguous; define units or usegoogle.protobuf.Timestamp.
uint64 TimeStamp = 13;needs an explicit unit/epoch to prevent producer/consumer drift (seconds vs ms vs ns). Prefergoogle.protobuf.Timestamp, or at minimum add a comment like// unix epoch milliseconds.
🧹 Nitpick comments (2)
packages/shared/src/protoTypes.proto (1)
7-335: Proto3 naming + numeric types: consider future compatibility/precision now.
A lot of fields look like integers/enums (IDs, flags, state, counts) but are defined asfloat(e.g., various...Id,SystemState, GPS date parts). If these are truly integer-valued, encode asint32/uint32(orenum) to avoid precision loss and awkward TS types. Also,UpperCamelCasefield names are unconventional in protobuf and can surprise tooling/codegen;lower_snake_caseis the standard.README.md (1)
132-140: Use a non-bashfence for.envexamples to avoid implying it’s shell.
Minor docs polish: usedotenv(or plain) fencing so users don’t try to execute it.Also applies to: 144-146
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
README.md(4 hunks)package.json(1 hunks)packages/shared/src/protoTypes.proto(1 hunks)
🧰 Additional context used
🪛 Buf (1.61.0)
packages/shared/src/protoTypes.proto
3-3: Files with package "telemetry" must be within a directory "telemetry" relative to root but were in directory "packages/shared/src".
(PACKAGE_DIRECTORY_MATCH)
🔇 Additional comments (2)
README.md (2)
3-3: Good: wrap deployment URL in angle brackets.
Improves markdown autolinking consistency.
70-76: Server TOC anchor links look fixed; please confirm the targets exist indocs/SERVER.md.
These moved from in-README fragments to./docs/SERVER.md#..., which should address the prior MD051 issues, assuming headings match exactly.
| ```bash | ||
| yarn dev | ||
| ``` |
There was a problem hiding this comment.
Docs: types:generate section should mention prerequisites (protoc, plugin install).
Right now it reads like yarn types:generate “just works”; in practice devs may need protoc installed (or a pinned toolchain). Consider adding a one-liner noting required installs / versions.
Also applies to: 113-116, 120-125
🤖 Prompt for AI Agents
In README.md around lines 105-107 (and similarly update sections at 113-116 and
120-125), the docs for the types:generate step omit prerequisites; add a concise
note stating developers must have protoc installed (include minimum or pinned
version) and the required plugin(s) (e.g., protoc-gen-ts or
grpc_tools_node_protoc) or provide a pinned toolchain (homebrew/apt/npm package)
as an alternative, with a one-line example of install commands and the version
constraint so the command works reliably for contributors.
804bc98 to
e83f457
Compare
Summary by CodeRabbit
Documentation
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.