refactor(instance-settings): streamline modal state and save flow#4
refactor(instance-settings): streamline modal state and save flow#4
Conversation
Streamlined instance settings and fingerprint update flow to reduce state mismatch between UI and backend.
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 refactors the instance settings and fingerprint update flow to enhance user control over window geometry when auto-fingerprinting is active. The changes aim to reduce state mismatch between the UI and backend by providing explicit options for managing window size and position, thereby improving the overall consistency and user experience of instance configuration. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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. 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 effectively streamlines the instance settings, particularly for handling window size in the automatic fingerprint generation mode. The changes introduce a new option to either randomize the window size on each launch or use a deterministic size, with corresponding updates to the Rust backend and Svelte frontend. The implementation is largely solid, but I've identified a few opportunities to enhance code clarity and maintainability. My review includes suggestions for using named constants instead of magic numbers in the Rust code, refactoring a logic block in the Svelte component to be more concise, and moving inline styles to a dedicated CSS file to improve separation of concerns.
| let (outer_w, outer_h) = if change_window_size { | ||
| let pct_w = rng.gen_range(75..=100) as f64 / 100.0; | ||
| let pct_h = rng.gen_range(75..=100) as f64 / 100.0; | ||
| ( | ||
| ((avail_w as f64 * pct_w) as u32).max(800), | ||
| ((avail_h as f64 * pct_h) as u32).max(600), | ||
| ) | ||
| } else if let (Some(w), Some(h)) = (default_outer_width, default_outer_height) { | ||
| (w.min(avail_w), h.min(avail_h)) | ||
| } else { | ||
| ( | ||
| ((avail_w as f64 * 0.9) as u32).max(800), | ||
| ((avail_h as f64 * 0.9) as u32).max(600), | ||
| ) | ||
| }; |
There was a problem hiding this comment.
This block uses several magic numbers (75..=100, 800, 600, 0.9). To improve readability and maintainability, consider defining them as named constants at the top of the module. This makes the code self-documenting and easier to modify in the future.
For example:
const MIN_WINDOW_DIM_PCT: u8 = 75;
const MAX_WINDOW_DIM_PCT: u8 = 100;
const MIN_OUTER_WIDTH: u32 = 800;
const MIN_OUTER_HEIGHT: u32 = 600;
const DEFAULT_WINDOW_SCALE: f64 = 0.9;| fp.auto_change_window_size = autoMode ? autoChangeWindowSize : null; | ||
| if (autoMode && !autoChangeWindowSize && autoWindowPresetIndex >= 0 && WINDOW_PRESETS[autoWindowPresetIndex]) { | ||
| const preset = WINDOW_PRESETS[autoWindowPresetIndex]; | ||
| fp.outer_width = preset.w; | ||
| fp.outer_height = preset.h; | ||
| fp.inner_width = null; | ||
| fp.inner_height = null; | ||
| } else if (autoMode) { | ||
| fp.outer_width = null; | ||
| fp.outer_height = null; | ||
| fp.inner_width = null; | ||
| fp.inner_height = null; | ||
| } |
There was a problem hiding this comment.
This logic for setting window dimensions in AUTO mode can be refactored to be more concise and avoid repetition. The fp.inner_width = null and fp.inner_height = null assignments are made in both branches of the conditional. You can simplify this by checking for autoMode once and then handling the conditional logic for outer_width and outer_height inside that block, setting the inner dimensions just once at the end.
fp.auto_change_window_size = autoMode ? autoChangeWindowSize : null;
if (autoMode) {
const preset = !autoChangeWindowSize && autoWindowPresetIndex >= 0 ? WINDOW_PRESETS[autoWindowPresetIndex] : null;
if (preset) {
fp.outer_width = preset.w;
fp.outer_height = preset.h;
} else {
fp.outer_width = null;
fp.outer_height = null;
}
fp.inner_width = null;
fp.inner_height = null;
}
| <div style="margin-top: 10px; padding-top: 10px; border-top: 1px solid rgba(16, 185, 129, 0.2); display: flex; align-items: center; justify-content: space-between; gap: 12px;"> | ||
| <div style="display: flex; flex-direction: column; gap: 4px;"> | ||
| <span style="font-size: 0.58rem; letter-spacing: 0.08em; color: var(--text-primary);">CHANGE WINDOW SIZE EACH LAUNCH</span> | ||
| <span style="font-size: 0.55rem; letter-spacing: 0.03em; color: var(--text-secondary);"> | ||
| {autoChangeWindowSize | ||
| ? 'Randomizes window size and position every launch.' | ||
| : 'Keeps window size and position deterministic for the selected profile.'} | ||
| </span> | ||
| </div> | ||
| <button | ||
| class="btn {autoChangeWindowSize ? 'btn-active' : ''}" | ||
| style="font-size: 0.58rem; padding: 4px 10px; min-width: 50px; letter-spacing: 0.05em; {autoChangeWindowSize ? `background: ${accentGreen}; color: #000; border-color: ${accentGreen};` : ''}" | ||
| on:click={() => { autoChangeWindowSize = !autoChangeWindowSize; }} | ||
| > | ||
| {autoChangeWindowSize ? 'ON' : 'OFF'} | ||
| </button> | ||
| </div> | ||
| <div style="margin-top: 10px; display: flex; flex-direction: column; gap: 6px; opacity: {autoChangeWindowSize ? 0.6 : 1};"> | ||
| <label for="auto-window-size-preset" style="font-size: 0.58rem; letter-spacing: 0.08em; color: var(--text-primary);">DEFAULT WINDOW SIZE PRESET</label> | ||
| <select | ||
| id="auto-window-size-preset" | ||
| class="input-field" | ||
| style="font-size: 0.65rem;" | ||
| bind:value={autoWindowPresetIndex} | ||
| disabled={autoChangeWindowSize} | ||
| > | ||
| <option value={-1}>AUTO DEFAULT</option> | ||
| {#each WINDOW_PRESETS as p, i} | ||
| <option value={i}>{p.label}</option> | ||
| {/each} | ||
| </select> | ||
| <span style="font-size: 0.54rem; letter-spacing: 0.03em; color: var(--text-secondary);"> | ||
| Applies when window-size changing is OFF. | ||
| </span> | ||
| </div> |
There was a problem hiding this comment.
This section contains a lot of inline styles. For better maintainability, readability, and separation of concerns, it's recommended to move these styles into the InstanceSettingsModal.css file and use CSS classes instead. This also allows for easier reuse of styles and keeps the HTML markup cleaner. For example, you could create classes like .auto-mode-option and .auto-mode-description for the styled elements.
1 participant
Streamlined instance settings and fingerprint update flow to reduce state mismatch between UI and backend.