Skip to content

fix(settings): bind settings dialog open state with v-model for Vue 3 - #3577

Open
jay23606 wants to merge 1 commit into
nextcloud:masterfrom
jay23606:pr-go/3549-reopening-the-settings-is-not-possible
Open

jay23606 wants to merge 1 commit into
nextcloud:masterfrom
jay23606:pr-go/3549-reopening-the-settings-is-not-possible

Conversation

@jay23606

@jay23606 jay23606 commented Nov 9, 2026

Copy link
Copy Markdown

Bug

The settings dialog cannot be reopened after it has been closed once (#3549). SettingsDialog.vue binds the dialog's open state with :open.sync, which is Vue 2 syntax. Vue 3 ignores .sync, so the update:open event from NcAppSettingsDialog never updates isOpen.

Change

Replace :open.sync="isOpen" with v-model:open="isOpen" in src/components/Modals/SettingsDialog.vue. The regression test tests/check_settings_dialog_model.py fails on the base binding and checks that NcAppSettingsDialog declares the update:open event that v-model:open relies on.

Testing

  • python tests/check_settings_dialog_model.py passes on this change (OK: SettingsDialog.vue binds the open state with v-model:open).
  • The same test run against the base version of SettingsDialog.vue fails with FAIL: SettingsDialog.vue uses ':open.sync', which Vue 3 ignores; expected 'v-model:open'.
  • Not verified: the reopen behaviour in a running Nextcloud instance, and the full test suite.

Regression test: fails on base twice (/FAIL: SettingsDialog.vue uses ':open.sync'/), passes on fix twice (python tests/check_settings_dialog_model.py)

Fixes #3549

@coderabbitai

coderabbitai Bot commented Nov 9, 2026 •

Copy link
Copy Markdown

Explain Change →

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: e4a8422b-329b-4c9d-85fe-99c6c9813eba
Commits

Reviewing files that changed from the base of the PR and between 422acef and 48107b9.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • Trigger review
Walkthrough

The settings dialog changes its open-state binding from Vue 2’s .sync syntax to Vue 3’s v-model:open syntax. A new test script checks for the updated binding and the dialog’s update:open declaration.

Severity of issue fixed: Medium

Merge Risk: Minimal · previous review 422ac

The binding change has no established merge-blocking defect. Automated close-and-reopen coverage would improve confidence.

Pre-merge checks: Passed 5

Check name Status Explanation
Title check Passed The title clearly and concisely describes the Vue 3 settings dialog binding change.
Description check Passed The description explains the bug, the code change, the regression test results, and the runtime testing that remains unverified. It does not include the template’s “Concerns/issues” or “Formal require…
Linked Issues check Passed Issue [#3549] identifies the Vue 3 named-model binding as the cause of the settings dialog reopening problem. The change replaces :open.sync="isOpen" with v-model:open="isOpen" in `SettingsDialog.…
Out of Scope Changes check Passed The only changes are the SettingsDialog.vue binding fix and a regression test for that binding. Both directly support issue [#3549]. No unrelated changes appear in the whole-PR diff.
Docstring Coverage Passed No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Full details: Description check

Explanation

The description explains the bug, the code change, the regression test results, and the runtime testing that remains unverified. It does not include the template’s “Concerns/issues” or “Formal requirements” sections, but the core information is present.

Full details: Linked Issues check

Explanation

Issue [#3549] identifies the Vue 3 named-model binding as the cause of the settings dialog reopening problem. The change replaces :open.sync="isOpen" with v-model:open="isOpen" in SettingsDialog.vue. The added regression test checks the binding and the dialog's update:open event declaration. This addresses the issue's coding requirement.

Full details: Docstring Coverage

Explanation

No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 1 files. (1 skipped: 1 unsupported.)

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nitpick comments: 2

tests/check_settings_dialog_model.py: 2

13-17: Trivial · Maintainability & Code Quality · Quick win

Run this check through the standard test commands.

npm test and npm run ci-test invoke only Vitest, so they do not run this Python script. A binding regression can therefore pass those commands. Port the check to Vitest or add the script to the test commands. (raw.githubusercontent.com)


18-26: Trivial · Maintainability & Code Quality · Quick win

Add a close-and-reopen regression test

The string checks do not exercise update:open or the parent’s state reset. A regression in either path can pass this script while settings cannot reopen. Add a behavioral test that opens the dialog, closes it through the child, and opens it again.

Review info

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: df0550b8-e21b-499a-bd07-2cc516711c2f
Commits

Reviewing files that changed from the base of the PR and between eb49783 and 422acef.

Files selected for processing: 2

  • src/components/Modals/SettingsDialog.vue
  • tests/check_settings_dialog_model.py

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

Signed-off-by: jay23606 <4602056+jay23606@users.noreply.github.com>
@jay23606
jay23606 force-pushed the pr-go/3549-reopening-the-settings-is-not-possible branch from 422acef to 48107b9 Compare November 9, 2026 20:35
@christianlupus

Copy link
Copy Markdown
Collaborator

What is this Python thing about?

@christianlupus

Copy link
Copy Markdown
Collaborator

Is an pretty sure, this is AI generated. Please adhere to the code of conduct regarding AI based contributions.

@jay23606

jay23606 commented Nov 9, 2026

Copy link
Copy Markdown
Author

The file is a regression test, tests/check_settings_dialog_model.py. It reads SettingsDialog.vue and fails if it still uses ':open.sync' or lacks 'v-model:open="isOpen"', and it checks that NcAppSettingsDialog declares the 'update:open' event.

@jay23606

jay23606 commented Nov 9, 2026

Copy link
Copy Markdown
Author

This round did not change the code or the PR description. The PR description does not yet include the AI tool use disclosure that the Nextcloud AI Contribution Policy requires, and that disclosure has to be added by the contributor.

@christianlupus

Copy link
Copy Markdown
Collaborator

Drop the apparent test file. This will neither be called nor is it a real test.

This branch has not been deployed

No deployments
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.

Reopening the settings is not possible

2 participants