Skip to content

Conversation

@hsorby
Copy link
Contributor

@hsorby hsorby commented Dec 18, 2025

No description provided.

@hsorby hsorby requested a review from agarny December 19, 2025 02:01
return response
}
defineExpose({ getDataView })
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why getDataView? Why not simply getData?

</template>

<script setup>
const emit = defineEmits(['data-ready'])
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would use data-available (or dataAvailable).

Copy link
Collaborator

Choose a reason for hiding this comment

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

In fact, this emit is not used, so why define it?

Comment on lines 62 to 68
if (type === 'sine') {
// Smooth curve.
data.push(Math.sin(i * 0.1) * 10)
} else {
// Random noise.
data.push(Math.random() * 10)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might want to add some mock data for the variable of integration?

Comment on lines 86 to 97
// Check request is expected type.
if (req.id !== EXPECTED_ID) {
console.warn(`[Mock] Request #${index} ignored: Invalid ID '${req.id}'`)
return // Skip this specific item.
}
// Test version compatibility.
const requestMajorVersion = parseInt(req.version.split('.')[0])
if (requestMajorVersion !== EXPECTED_MAJOR_VERSION) {
console.warn(`[Mock] Request #${index} ignored: Version mismatch. Expected v${EXPECTED_MAJOR_VERSION}.x, got v${req.version}`)
return // Skip this specific item.
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wouldn't bother with this, but... meh.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Still wouldn't bother with this kind of check.

if (req.variable && req.variable.includes('v_in')) {
response[req.identifier] = generateMockSeries('sine')
} else {
// Default fallback for other variables.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also come here if req.variable is not truthy.

} else {
// Default fallback for other variables.
response[req.identifier] = generateMockSeries('random')
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would expect req.variable to be something like membrane/V or sodium_current/i_Na. As for the variable of integration, we could use VOI as a special variable name. So, here, I would expect to add a case for where req.variable === 'VOI'?

Copy link
Collaborator

@agarny agarny left a comment

Choose a reason for hiding this comment

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

It would be nice to have a PR that doesn't consist of 99% of reformatted code. It makes the PR unnecessarily difficult to review.

This aside, I understand that you expose getData():

  • I would probably called that method getSimulationData().
  • It's not clear to me what the requests parameter is supposed to look like.

if (!requests || !Array.isArray(requests)) {
console.error("getDataView: Request must be an array.")
return {};
console.error('getData: Request must be an array.')
Copy link
Collaborator

Choose a reason for hiding this comment

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

"request", not "Request".

function generateMockSeries(type, length = 100) {
const data = []
for (let i = 0; i < length; i++) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Very minor since this is bound to be removed, but: ++i rather than i++.

Comment on lines 86 to 97
// Check request is expected type.
if (req.id !== EXPECTED_ID) {
console.warn(`[Mock] Request #${index} ignored: Invalid ID '${req.id}'`)
return // Skip this specific item.
}
// Test version compatibility.
const requestMajorVersion = parseInt(req.version.split('.')[0])
if (requestMajorVersion !== EXPECTED_MAJOR_VERSION) {
console.warn(`[Mock] Request #${index} ignored: Version mismatch. Expected v${EXPECTED_MAJOR_VERSION}.x, got v${req.version}`)
return // Skip this specific item.
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still wouldn't bother with this kind of check.

</template>

<script setup>
const emit = defineEmits(['data-ready'])
Copy link
Collaborator

Choose a reason for hiding this comment

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

In fact, this emit is not used, so why define it?

requests.forEach((req, index) => {
// --- VALIDATION BLOCK ---
// Check request is expected type.
Copy link
Collaborator

Choose a reason for hiding this comment

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

// Check that the request is of the expected type.

const EXPECTED_ID = 'nz.ac.auckland.simulation-data-request'
const EXPECTED_MAJOR_VERSION = 0
requests.forEach((req, index) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not 100% clear to me what requests is expected to look like.

// Check request is expected type.
if (req.id !== EXPECTED_ID) {
console.warn(`[Mock] Request #${index} ignored: Invalid ID '${req.id}'`)
Copy link
Collaborator

Choose a reason for hiding this comment

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

"invalid", not "Invalid". Also need a final stop.

Comment on lines +128 to +130
if (req.position) {
response.position = { ...req.position }
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the point of req.position? It is in the request and then put in the response without doing anything with it in between?

response.position = { ...req.position }
}
if (!response['data']) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this test always true since we start with response = {}?

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.

2 participants