Skip to content

Fix model selection on index 0#1955

Closed
yuhao1118 wants to merge 1 commit intoPhotonVision:mainfrom
ATOM-STORM-6766:fix/model-select-dropdown
Closed

Fix model selection on index 0#1955
yuhao1118 wants to merge 1 commit intoPhotonVision:mainfrom
ATOM-STORM-6766:fix/model-select-dropdown

Conversation

@yuhao1118
Copy link

Description

Fixed model selection bug where selecting the first item in the dropdown failed to update settings, as index 0 was implicitly converted to false in the condition check.

Meta

Merge checklist:

  • Pull Request title is short, imperative summary of proposed changes
  • The description documents the what and why
  • If this PR changes behavior or adds a feature, user documentation is updated
  • If this PR touches photon-serde, all messages have been regenerated and hashes have not changed unexpectedly
  • If this PR touches configuration, this is backwards compatible with settings back to v2024.3.1
  • If this PR touches pipeline settings or anything related to data exchange, the frontend typing is updated
  • If this PR addresses a bug, a regression test for it is added

@yuhao1118 yuhao1118 requested a review from a team as a code owner May 7, 2025 15:35
@mcm001 mcm001 requested a review from samfreund May 7, 2025 15:36
@samfreund
Copy link
Member

Could you give me some more information about the bug happening here? I don't seem to be able to reproduce it.

@yuhao1118
Copy link
Author

Could you give me some more information about the bug happening here? I don't seem to be able to reproduce it.

When selecting the first model in the dropdown list, the UI fails to update the selected value because index 0 is implicitly converted to false in the conditional check, preventing the update logic from executing.

I noticed that PR #1954 also addresses this issue, so I'm fine with whichever pr you'd choose to merge. 😄

@samfreund
Copy link
Member

Could you give me some more information about the bug happening here? I don't seem to be able to reproduce it.

When selecting the first model in the dropdown list, the UI fails to update the selected value because index 0 is implicitly converted to false in the conditional check, preventing the update logic from executing.

I noticed that PR #1954 also addresses this issue, so I'm fine with whichever pr you'd choose to merge. 😄

Got it, I think I see what's up now. I was testing on 2025.3.1 which is why I didn't notice. We've been going through a big dependencies upgrade (including Vue 3), which is probably where that's coming from. Nice job picking up on this, but we'll probably do it thru #1954. Part of the reason it didn't get caught earlier is because that logic is likely going to be changing soon, so it wasn't the highest priority for testing. Thanks for the quick fix tho, those are important!

@samfreund samfreund closed this May 7, 2025
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