#5987
Pull Requests/lizardbyte/Sunshine/#5987

fix(web): save an app that has no cover image

Openflashbosswants to merge intofrom
1 commit3 files changed+28-3

Description

Saving an application with no image used to stop, so nothing was stored. The save now goes through. No new cover is created: the app keeps the initial letter already shown in the list.

Screenshot

Issues Fixed or Closed

Roadmap Issues

Testing

Type of Change

  • feat: New feature (non-breaking change which adds functionality)
  • fix: Bug fix (non-breaking change which fixes an issue)
  • docs: Documentation only changes
  • style: Changes that do not affect the meaning of the code (white-space, formatting, missing semicolons, etc.)
  • refactor: Code change that neither fixes a bug nor adds a feature
  • perf: Code change that improves performance
  • test: Adding missing tests or correcting existing tests
  • build: Changes that affect the build system or external dependencies
  • ci: Changes to CI configuration files and scripts
  • chore: Other changes that don't modify src or test files
  • revert: Reverts a previous commit
  • BREAKING CHANGE: Introduces a breaking change (can be combined with any type above)

Checklist

  • Code follows the style guidelines of this project
  • Code has been self-reviewed
  • Code has been commented, particularly in hard-to-understand areas
  • Code docstring/documentation-blocks for new or existing methods/components have been added or updated
  • Tests have been added or updated for new or modified functionality, including regression tests for bug fixes

AI Usage

See our AI usage policy.

  • AI tools were used while preparing this PR

Activity

5
L
fix(web): save an app that has no cover image
ae9b4dc
flashboss5d ago

Could a maintainer please review it? By fixing it, you’d save yourself quite a few headaches during configuration.

flashboss2d ago

Hi, welcome!

Saving with "image-path": "" (empty string) already works, as in your example.

The failing case was different: image-path missing / null / undefined on the form (new app, or cover cleared without leaving ""), not an empty string.

Before the fix, save() did:

this.editForm["image-path"].toString().replaceAll('"', '') With null/undefined, .toString() throws and saving stops in the UI (no successful submit).

Also, even with "", the backend could persist "image-path": "" in apps.json. The fix:

accepts a missing path without crashing; omits the key when empty, so the initial-letter placeholder remains (same as Desktop without a custom cover). Reproduction: Applications → Add → name only, no cover / Find cover → Save. If image-path is not initialized to "", saving fails in the console/UI without this fix.

ReenigneArcher2d ago

Please expand this PR to handle missing/null values consistently for all optional application fields, including the cover image. This is an explicit scope request for the fix; the other null-handling problems predate this PR.

  • Normalize the editor state before rendering: prep-cmd and detached default to []; cmd, output, working-dir, and image-path default to ""; the four flags and exit-timeout use their established defaults. Use nullish handling so explicit false and 0 survive.
  • Apply the same missing/null contract at the save endpoint for direct API callers: omit null-valued optional fields before persisting them, and include the optional do, undo, and elevated members of preparation-command entries. Keep required name/index handling separate, and preserve valid configured values.
  • Extend the regression tests and Doxygen documentation to cover that contract. The existing backend test suggestion can be expanded to check create/update requests with each optional field absent or null, plus explicit false, zero timeout, empty collections, and configured strings/commands.

On head ae9b4dc111be15d9c1c40d3b1f244aad11ef623b, the additional missing/null editor cases fail and the preservation case passes. All three pass with the editor example below in an isolated review copy. The backend/nested-field changes have not been implemented or tested locally.

Tested editor example and concrete Vitest regression tests

In src_assets/common/assets/web/Apps.vue, replace the existing editApp() method with:

      /**
       * @brief Open an application for editing with nullish optional fields defaulted.
       * @param {number} id Index of the application to edit.
       */
      editApp(id) {
        this.editForm = structuredClone(toRaw(this.apps[id]));
        this.editForm.index = id;
        const defaults = {
          "prep-cmd": [],
          detached: [],
          "exclude-global-prep-cmd": false,
          elevated: false,
          "auto-detach": true,
          "wait-all": true,
          "exit-timeout": 5,
          cmd: "",
          output: "",
          "working-dir": "",
          "image-path": ""
        };
        for (const [key, value] of Object.entries(defaults)) {
          this.editForm[key] ??= value;
        }
        this.editFormError = "";
        this.openEditModal();
      },

Add tests/web/Apps.optional-fields.test.js:

import { beforeEach, describe, expect, it, vi } from 'vitest'

vi.mock('../../src_assets/common/assets/web/Navbar.vue', () => ({
  default: { template: '<div />' },
}))
vi.mock('../../src_assets/common/assets/web/fetch_utils', () => ({
  apiFetch: vi.fn(() => Promise.resolve({ status: 204 })),
}))

import { apiFetch } from '../../src_assets/common/assets/web/fetch_utils'
import Apps from '../../src_assets/common/assets/web/Apps.vue'

beforeEach(() => vi.clearAllMocks())

/**
 * @brief Construct editing state without invoking Bootstrap or the network.
 * @param {object} app Application JSON to edit.
 * @return {object} Context for editApp() and save().
 */
function editContext(app) {
  return {
    apps: [app],
    platform: 'windows',
    openEditModal: vi.fn(),
    $refs: { editModal: { querySelector: () => ({ scrollTop: 0 }) } },
    $t: key => key,
  }
}

describe('optional application fields', () => {
  const defaults = {
    'prep-cmd': [],
    detached: [],
    'exclude-global-prep-cmd': false,
    elevated: false,
    'auto-detach': true,
    'wait-all': true,
    'exit-timeout': 5,
    cmd: '',
    output: '',
    'working-dir': '',
    'image-path': '',
  }

  it.each([undefined, null])('defaults optional fields containing %s before editing and saving', value => {
    const app = { name: 'No cover' }
    if (value === null) {
      for (const key of Object.keys(defaults)) app[key] = null
    }
    const ctx = editContext(app)

    Apps.methods.editApp.call(ctx, 0)

    expect(ctx.editForm).toEqual({ name: 'No cover', index: 0, ...defaults })
    expect(() => Apps.methods.addPrepCmd.call(ctx)).not.toThrow()
    expect(() => Apps.methods.addDetached.call(ctx)).not.toThrow()
    expect(() => Apps.methods.save.call(ctx)).not.toThrow()
    expect(apiFetch).toHaveBeenCalledOnce()
    expect(Object.values(JSON.parse(apiFetch.mock.calls[0][1].body))).not.toContain(null)
  })

  it('preserves false, zero, and configured optional values', () => {
    const app = {
      name: 'Configured',
      ...defaults,
      'prep-cmd': [{ do: 'prepare', undo: 'restore', elevated: false }],
      detached: ['background'],
      'auto-detach': false,
      'wait-all': false,
      'exit-timeout': 0,
      cmd: 'launch --argument',
      output: '/logs/app.log',
      'working-dir': '/apps',
      'image-path': '/covers/cover.png',
    }
    const ctx = editContext(app)

    Apps.methods.editApp.call(ctx, 0)
    Apps.methods.save.call(ctx)

    expect(ctx.editForm).toEqual({ ...app, index: 0 })
    expect(JSON.parse(apiFetch.mock.calls[0][1].body)).toEqual({ ...app, index: 0 })
    expect(app).not.toHaveProperty('index')
  })
})

Run from the PR checkout on Windows:

& 'C:\msys64\msys2_shell.cmd' -defterm -here -no-start -ucrt64 -c '/c/Progra~1/nodejs/node.exe node_modules/vitest/vitest.mjs run tests/web/Apps.optional-fields.test.js'

Review completed by Codex | Model: gpt-6.1-sol | Thinking: xhigh