-
Notifications
You must be signed in to change notification settings - Fork 129
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: circuitname in property panel #452
base: main
Are you sure you want to change the base?
fix: circuitname in property panel #452
Conversation
WalkthroughThe pull request updates several Vue components and store modules to improve circuit name handling and state management. The changes refactor the input logic in components such as MessageBox and ProjectProperty to use computed properties and store-driven approaches. The modifications include correcting property name typos in the Properties Panel components, updating event payloads, and adjusting store initializations and type definitions. Additionally, helper components and the circuit creation logic now retrieve default naming values from the prompt store, while logging and focus management are refined. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MB as MessageBox.vue
participant PP as ProjectProperty.vue
participant PC as PromptComponent.vue
participant Store as SimulatorStore
participant Circuit as circuit.ts
User->>MB: Mount component (auto-focus input)
MB->>Store: Retrieve circuitName (computed)
User->>PP: Update circuit name via input (v-model binding)
PP->>Store: Update activeCircuit state
User->>PC: Click button ("save")
PC->>Store: Emit event with circuitItem (updated name)
PC->>Circuit: Invoke createNewCircuitScope (using promptStore input)
Circuit->>Store: Update activeCircuit and log circuit switch
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
Tip 🌐 Web search-backed reviews and chat
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for circuitverse ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
src/components/Panels/PropertiesPanel/ModuleProperty/ModuleProperty.vue (1)
1-43
: Issue found in parent component bindingThe search reveals that the parent component(s) (specifically in both v0 and v1 under
PropertiesPanel.vue
) are still using the incorrect binding:panle-body-data
. This needs to be updated to:panel-body-data
to match the intended property name in the child (ModuleProperty.vue
).
- v0/src/components/Panels/PropertiesPanel/PropertiesPanel.vue: Update
:panle-body-data
to:panel-body-data
.- v1/src/components/Panels/PropertiesPanel/PropertiesPanel.vue: Update
:panle-body-data
to:panel-body-data
.🔗 Analysis chain
Verify parent component property binding.
The property name is now correctly defined as
panelBodyData
. Please ensure that all parent components using this component have updated their bindings from:panle-body-data
to:panel-body-data
.
I'm now generating a new shell script to search for any occurrence of the old property naming pattern in all Vue files by using a glob pattern.
I'm running an additional script to view the context of the matching lines in the affected Vue files so we can inspect whether the old property names are still in use and need to be updated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining instances of the old property name rg -l "panle-body-data|panleBodyData" --type vueLength of output: 78
Script:
#!/bin/bash # Search for any remaining instances of the old property name in .vue files using a glob pattern rg -l -g '*.vue' 'panle-body-data|panleBodyData'Length of output: 322
Script:
#!/bin/bash # Display context for each occurrence of the old property names in the Vue files. for file in $(rg -l -g '*.vue' 'panle-body-data|panleBodyData'); do echo "=== $file ===" rg -n -C 2 'panle-body-data|panleBodyData' "$file" echo "" doneLength of output: 3465
🧰 Tools
🪛 ESLint
[error] 33-33: 'ref' is defined but never used.
(no-unused-vars)
[error] 34-34: 'onMounted' is defined but never used.
(no-unused-vars)
🧹 Nitpick comments (5)
src/store/propertiesPanelStore.ts (1)
8-8
: Consider using a more specific type instead ofany
.Using
any
type bypasses TypeScript's type checking benefits. Consider defining an interface for the expected object structure to maintain type safety.-const propertiesPanelObj:any = ref() +interface PropertiesPanelObject { + // Define expected properties here +} +const propertiesPanelObj = ref<PropertiesPanelObject | null>(null)src/store/SimulatorStore/state.ts (1)
8-8
: Consider makingname
required since it's initialized with a default value.The type definition makes
name
optional (string | undefined
) but it's always initialized with an empty string. This creates an inconsistency between the type definition and actual usage.-name: string | undefined +name: stringsrc/components/MessageBox/messageBox.vue (1)
41-41
: Consider using a more descriptive object structure for the event payload.The event payload could be more explicit about its purpose.
-@click="$emit('buttonClick', buttonItem.emitOption, {name:circuitName})" +@click="$emit('buttonClick', buttonItem.emitOption, {circuitName: circuitName})"src/components/helpers/promptComponent/PromptComponent.vue (1)
149-158
: Add type safety to the confirmation handler.The
circuitItem
parameter is typed asany
, which loses type safety. Consider creating an interface for the circuit item structure.+interface CircuitItem { + id: string; + name: string; +} -const confirmation = (selectedOption: string, circuitItem:any): void => { +const confirmation = (selectedOption: string, circuitItem: CircuitItem): void => {src/simulator/src/circuit.ts (1)
190-195
: Improve error handling in createNewCircuitScope.The function should handle the case where
provideCircuitName()
returns undefined.const promptStore = usePromptStore() name = await provideCircuitName(); - if (name instanceof Error) return // if user cancels the prompt - if (name.trim() == '') { + if (name instanceof Error || !name || name.trim() === '') { name = promptStore.prompt.inputText; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/components/MessageBox/messageBox.vue
(1 hunks)src/components/Panels/PropertiesPanel/ModuleProperty/ModuleProperty.vue
(2 hunks)src/components/Panels/PropertiesPanel/ModuleProperty/ProjectProperty/ProjectProperty.vue
(3 hunks)src/components/Panels/PropertiesPanel/PropertiesPanel.vue
(1 hunks)src/components/helpers/promptComponent/PromptComponent.vue
(5 hunks)src/simulator/src/circuit.ts
(4 hunks)src/store/SimulatorStore/state.ts
(3 hunks)src/store/promptStore.ts
(2 hunks)src/store/propertiesPanelStore.ts
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/components/Panels/PropertiesPanel/PropertiesPanel.vue
🔇 Additional comments (7)
src/store/SimulatorStore/state.ts (1)
48-48
: LGTM! Good initialization with empty strings.Initializing with empty strings instead of undefined is a good practice for reactive properties in Vue.
src/store/promptStore.ts (1)
9-10
: LGTM! Good default value for circuit name.The addition of
inputText
with a default value of 'Untitled-circuit' is a good practice for handling new circuits.Also applies to: 65-65
src/components/MessageBox/messageBox.vue (2)
57-62
: LGTM! Good use of computed property for two-way binding.The computed property with getter/setter provides clean two-way binding with the store.
64-71
: LGTM! Good focus management implementation.Using
nextTick
ensures the DOM is updated before attempting to focus, and selecting the text provides better UX.src/components/Panels/PropertiesPanel/ModuleProperty/ProjectProperty/ProjectProperty.vue (1)
119-126
: LGTM! Good use of computed property.The computed property implementation for the circuit name provides clean two-way data binding and proper type safety.
src/components/Panels/PropertiesPanel/ModuleProperty/ModuleProperty.vue (2)
13-14
: LGTM! Property name corrected consistently.The typo fix from
panleBodyData
topanelBodyData
is applied consistently across all component bindings. The:key
binding ensures proper component updates when the data changes.Also applies to: 18-19, 21-21
37-37
: LGTM! Property definition and destructuring updated correctly.The property definition and destructuring are properly updated to use
panelBodyData
. The type definition and reactivity handling withtoRefs
are maintained correctly.Also applies to: 42-42
src/components/Panels/PropertiesPanel/ModuleProperty/ProjectProperty/ProjectProperty.vue
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/helpers/promptComponent/PromptComponent.vue (1)
16-16
: Add proper typing for the circuit state.While the state management setup is correct, using
any
type reduces type safety. Consider defining an interface for the circuit item structure.+interface CircuitItem { + id: string; + name: string; + // Add other relevant circuit properties +} -const projectStore = useState(); +const projectStore = useState<{ + circuit_list: CircuitItem[]; + activeCircuit: CircuitItem | null; +}>();Also applies to: 68-68
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/Panels/PropertiesPanel/ModuleProperty/ProjectProperty/ProjectProperty.vue
(2 hunks)src/components/helpers/promptComponent/PromptComponent.vue
(5 hunks)src/simulator/src/circuit.ts
(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/simulator/src/circuit.ts
- src/components/Panels/PropertiesPanel/ModuleProperty/ProjectProperty/ProjectProperty.vue
🔇 Additional comments (2)
src/components/helpers/promptComponent/PromptComponent.vue (2)
9-9
: LGTM! Event handler updated correctly.The buttonClick event handler now properly passes both the selectedOption and circuitItem parameters to the confirmation method.
89-89
: LGTM! Consistent placeholder text.The placeholder text is now consistent with the circuit name input, improving user experience.
Fixes #408
Resolves the issue of circuitname not showing correctly in Properties panel.
Changes:
Reference Screenshot:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor