fix: Control Island stability and camera jitter resolution#1278
fix: Control Island stability and camera jitter resolution#1278dasilva333 wants to merge 3 commits intomoeru-ai:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the user experience by addressing several critical UI stability and interaction issues. It refines the behavior of the Control Island, preventing unexpected collapses and ensuring smooth sub-menu navigation. Furthermore, it eliminates camera jitter in the 3D model viewer and enhances window management with an "always on top" option and more robust dragging functionality across different operating systems. The changes also include improved logging for backend processes. Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several useful fixes for UI stability, camera jitter, and platform-specific window behavior. The refactoring of the Control Island's collapse logic and the introduction of a programmatic lock in OrbitControls are positive changes. However, a critical security vulnerability has been identified: the changes in apps/stage-tamagotchi/src/main/index.ts introduce logging of potentially sensitive information, such as provider configuration (which may contain API keys) and raw LLM output (which may contain PII), to the console. These logging statements must be removed or sanitized to prevent sensitive data exposure. Additionally, a bug in the new window dragging logic and a potential robustness issue in the camera control updates were found. Specific feedback and code suggestions are provided to address these issues.
| function startDraggingWindow() { | ||
| if (!isLinux.value) { | ||
| startDraggingWindowInvoke() | ||
| } | ||
| } |
There was a problem hiding this comment.
isLinux is an asynchronous function returned by useElectronEventaInvoke. Accessing isLinux.value is incorrect as it will be undefined, causing !isLinux.value to always be true. This will result in startDraggingWindowInvoke() being called on all platforms, which may not be the intended behavior. To correctly handle the asynchronous platform check, you should await the result of isLinux(). Making startDraggingWindow an async function is a direct way to fix this.
async function startDraggingWindow() {
if (!await isLinux()) {
startDraggingWindowInvoke()
}
}
| if (data.config && (data.valid || !data.reason?.includes('required'))) { | ||
| console.log(` └─ Config: ${JSON.stringify(data.config)}`) | ||
| } |
There was a problem hiding this comment.
The application logs the entire data.config object to the console. Provider configurations often contain sensitive information such as API keys, secrets, or other credentials. Logging this information can lead to credential leakage if the console output is captured in logs or viewed by unauthorized individuals.
| if (data.config && (data.valid || !data.reason?.includes('required'))) { | |
| console.log(` └─ Config: ${JSON.stringify(data.config)}`) | |
| } | |
| if (data.config && (data.valid || !data.reason?.includes('required'))) { | |
| // Sensitive configuration logging removed | |
| } |
| console.log(`${cyan}[LLM Final Output]${reset} Session: ${data.sessionId}`) | ||
| console.log(`----------------------------------------`) | ||
| console.log(data.text) | ||
| console.log(`----------------------------------------`) |
There was a problem hiding this comment.
The application logs the full text of LLM output to the console. Depending on the use case, LLM output may contain Personally Identifiable Information (PII) or other sensitive data processed by the model. Logging this information to the console can lead to data exposure.
| console.log(`${cyan}[LLM Final Output]${reset} Session: ${data.sessionId}`) | |
| console.log(`----------------------------------------`) | |
| console.log(data.text) | |
| console.log(`----------------------------------------`) | |
| console.log(`${cyan}[LLM Final Output]${reset} Session: ${data.sessionId}`) | |
| console.log(`----------------------------------------`) | |
| // Raw LLM output logging removed for privacy | |
| console.log(`----------------------------------------`) |
| watch(modelSize, (newSize) => { | ||
| if (!controls.value) | ||
| return | ||
| isProgrammatic = true | ||
| controls.value.minDistance = newSize.z | ||
| controls.value.maxDistance = newSize.z * 20 | ||
| controls.value.update() | ||
| isProgrammatic = false | ||
| }, { immediate: true, deep: true }) |
There was a problem hiding this comment.
The use of an isProgrammatic flag is a good approach to prevent update loops. However, if an error occurs during the programmatic update (e.g., inside controls.value.update()), the isProgrammatic flag will not be reset to false. This could leave the controls in a state where user input is permanently ignored. Using a try...finally block will ensure the flag is always reset, making the implementation more robust.
watch(modelSize, (newSize) => {
if (!controls.value)
return
isProgrammatic = true
try {
controls.value.minDistance = newSize.z
controls.value.maxDistance = newSize.z * 20
controls.value.update()
}
finally {
isProgrammatic = false
}
}, { immediate: true, deep: true })
3 participants
This PR addresses several UI stability and synchronization issues: