Skip to content

Conversation

@Luxxy-GF
Copy link
Contributor

@Luxxy-GF Luxxy-GF commented Dec 5, 2025

Summary

This PR introduces several enhancements to the File Manager, Instance Configuration, and Sidebar, along with critical bug fixes and performance optimizations.

Features Implemented

Instance Configuration

  • Configuration Editor: Added a full-featured configuration editor for instances, reusing the GeneralConfiguration component from the creation wizard.

  • Features:

    • View and edit instance configuration options.
    • Searchable configuration keys.
    • Visual indication of overridden values.
    • Reset to default/inherited values.
    • UI Improvements: Enhanced the layout to support full-height scrolling and consistent styling with the rest of the application.

    closes Instance Configuration Page #61

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements a full-featured instance configuration page that allows users to view and edit instance configuration options. The implementation reuses the existing GeneralConfiguration component from the instance creation wizard and includes features like searchable configuration keys, visual indication of overridden values, and the ability to reset to defaults.

Key Changes

  • Added instance configuration editor with save functionality
  • Enhanced layout with flexbox and overflow handling for full-height scrolling
  • Implemented new updateInstance API function for PATCH operations

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

File Description
app/(main)/instance/layout.tsx Modified layout to support full-height scrolling with flexbox and min-h-0 constraints
app/(main)/instance/configuration/page.tsx Implemented complete configuration page with state management, save functionality, and GeneralConfiguration component integration
app/(main)/instance/_lib/instance.ts Added new updateInstance function to handle PATCH requests for updating instance configuration

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

export default function ConfigurationPage() {
const searchParams = useSearchParams();
const instanceName = searchParams.get('name');
const { instance, isLoading, isError } = useInstance(instanceName);
Copy link

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

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

The useInstance hook returns a mutate function that should be used instead of importing the global mutate from SWR. Using the instance-specific mutate function is more idiomatic and ensures proper cache invalidation.

Change line 17 to destructure mutate:

const { instance, isLoading, isError, mutate } = useInstance(instanceName);

Then update line 44 to use this mutate function:

mutate();

This removes the need for the import on line 11.

Copilot uses AI. Check for mistakes.

setIsSaving(true);
try {
await updateInstance(instanceName, config, instance?.project);
Copy link

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

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

Type mismatch: instance?.project is string | undefined, but updateInstance expects string | null as the third parameter. While JavaScript treats these similarly in the conditional check, this is a type inconsistency.

Consider explicitly converting undefined to null:

await updateInstance(instanceName, config, instance?.project ?? null);
Suggested change
await updateInstance(instanceName, config, instance?.project);
await updateInstance(instanceName, config, instance?.project ?? null);

Copilot uses AI. Check for mistakes.
Comment on lines 15 to 17
const searchParams = useSearchParams();
const instanceName = searchParams.get('name');
const { instance, isLoading, isError } = useInstance(instanceName);
Copy link

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

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

The configuration page is fetching instance data directly using useSearchParams and useInstance, but the layout already provides this data through useInstanceContext. This causes redundant API calls and is inconsistent with other pages (backups, files, snapshots) which use the context.

Consider using the context instead:

import { useInstanceContext } from '../_context/instance';

export default function ConfigurationPage() {
  const { instance, isLoading, isError, mutate } = useInstanceContext();
  // Remove searchParams and useInstance imports

This will simplify the code and eliminate the duplicate instance fetch.

Copilot uses AI. Check for mistakes.
useEffect(() => {
if (instance?.config) {
// Filter out volatile keys if necessary, but GeneralConfiguration handles display filtering
setConfig(instance.config);
Copy link

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

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

The config state initialization should also reset the isDirty flag to false when syncing with new instance data. Currently, if the instance data is revalidated (e.g., after save), the config state is updated but isDirty remains in its current state. This could show the save button as disabled when it should be enabled, or vice versa.

Add setIsDirty(false); inside this useEffect to ensure the dirty state is reset when config is initialized or updated from the server.

Suggested change
setConfig(instance.config);
setConfig(instance.config);
setIsDirty(false);

Copilot uses AI. Check for mistakes.
config={config}
expandedConfig={instance.expanded_config}
onConfigChange={handleConfigChange}
instanceType={instance.type as 'container' | 'virtual-machine'}
Copy link

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

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

The type assertion instance.type as 'container' | 'virtual-machine' may fail silently if the instance type is an unexpected value. While the Instance interface allows type: 'container' | 'virtual-machine' | string, the GeneralConfiguration component expects only the two specific types.

Consider adding validation or a fallback: instanceType={(instance.type === 'virtual-machine' ? 'virtual-machine' : 'container')}

Suggested change
instanceType={instance.type as 'container' | 'virtual-machine'}
instanceType={instance.type === 'virtual-machine' ? 'virtual-machine' : 'container'}

Copilot uses AI. Check for mistakes.
Comment on lines 38 to 42
export async function updateInstance(
name: string,
config: Record<string, string>,
project: string | null = null,
) {
Copy link

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

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

The updateInstance function lacks documentation describing its purpose, parameters, and return value. Consider adding a JSDoc comment explaining:

  • What this function does (updates an instance's configuration)
  • The expected format of the config parameter
  • The optional project parameter and when it's needed
  • What happens on success (void return)
  • What exceptions it throws

Example:

/**
 * Updates an instance's configuration via PATCH request.
 * @param name - The name of the instance to update
 * @param config - Key-value pairs of configuration options to update
 * @param project - Optional project name if the instance belongs to a specific project
 * @throws Error if the update fails with details from the API response
 */

Copilot uses AI. Check for mistakes.
@Hye-Dev Hye-Dev self-requested a review December 28, 2025 22:01
Copy link
Member

@Hye-Dev Hye-Dev left a comment

Choose a reason for hiding this comment

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

  1. The behavior of "overridden" seems incorrect. Overridden properties are properties that are modified from profile inheritance.

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.

Instance Configuration Page

2 participants