Skip to content

Conversation

@riedel
Copy link
Member

@riedel riedel commented Jul 13, 2025

Pull Request Overview

This PR enhances the Predictor by adding a device-time option, refactors data point handling, and updates build and export configurations.

Renaming addDatapoint to addDataPoint and changing parameter order is a breaking change but as Predictor hasn't been much in use, I only changed the examples.

File Description
src/predictor/index.js Added useDeviceTime, renamed addDataPoint, implemented timestamp checks and errors
src/index.js Updated addDataPoint signature, corrected JSDoc, and included Predictor export
rollup.config.mjs Activated copy plugin for WASM asset
package.json Updated Rollup dependency to ^2.79.2

@riedel riedel requested review from Copilot and tk-king July 13, 2025 08:17

This comment was marked as outdated.

@riedel riedel force-pushed the bundle-predictor branch from e743746 to 7311064 Compare July 13, 2025 08:26
@riedel riedel requested a review from Copilot July 13, 2025 08:34

This comment was marked as outdated.

@riedel riedel force-pushed the bundle-predictor branch from 7311064 to d70d02b Compare July 13, 2025 08:44
@riedel riedel force-pushed the bundle-predictor branch from d70d02b to 811de1b Compare July 13, 2025 08:45
@riedel riedel requested a review from Copilot July 14, 2025 06:34
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enhances timestamp handling and usability of the Predictor, updates build tooling, and exports the Predictor at the package root.

  • Introduces a useDeviceTime flag, renames and reorders addDataPoint parameters, and adds timestamp validation and rounding in Predictor
  • Exports Predictor from the root src/index.js
  • Reactivates the Rollup copy plugin, bumps Rollup version, and updates example code to use the new API

Reviewed Changes

Copilot reviewed 7 out of 12 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/predictor/index.js Added useDeviceTime, renamed addDataPoint, timestamp checks
src/index.js Exported Predictor, updated addDataPoint JSDoc and usage
rollup.config.mjs Un-commented and configured copy plugin for WASM assets
package.json Bumped Rollup dependency to ^2.79.2
examples/example-shake/timeModeExample.js Updated parameter order in example (method name typo remains)
examples/example-prediction-web/index.html Renamed addDatapoint to addDataPoint, updated signatures
examples/example-prediction-node/predictionExample.js Renamed addDatapoint to addDataPoint, updated signatures
Comments suppressed due to low confidence (3)

src/predictor/index.js:35

  • [nitpick] The JSDoc description for useDeviceTime contradicts the flag name. It should clarify that true uses device-generated timestamps (e.g., Date.now()) rather than server-provided ones.
     * @param {boolean} useDeviceTime - True if you want to use timestamps generated by the server

src/predictor/index.js:66

  • [nitpick] The JSDoc for addDataPoint only documents time; please add @param {string} sensorName and @param {number} value to fully describe the method signature.
     * @param {number} time - The timestamp assigned to the datapoint

src/index.js:85

  • [nitpick] The JSDoc for addDataPoint is missing an @returns tag. Please add @returns {Promise} or the appropriate return type to document the function's output.
   * Uploads a value for a specific timestamp to a dataset's timeSeries with name sensorName

const time = parseInt(ti);
for (const [key, valStr] of Object.entries(valObjs)) {
p.addDatapoint(key, parseInt(valStr), time)
p.addDatapoint(time,key, parseInt(valStr))
Copy link

Copilot AI Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The example still calls addDatapoint instead of the updated addDataPoint. Update the method name to p.addDataPoint(time, key, parseInt(valStr)).

Suggested change
p.addDatapoint(time,key, parseInt(valStr))
p.addDataPoint(time, key, parseInt(valStr))

Copilot uses AI. Check for mistakes.
(and fix some other occurences)
@riedel riedel force-pushed the bundle-predictor branch from 08fb4e4 to 450f827 Compare July 14, 2025 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Predictor not bundled addDataPoint in Collector vs addDatapoint in predictor have different signatures

1 participant