[fix] keep a settings save message on the card that saved - #137
Merged
Conversation
Both cards on the Settings page write through one endpoint, so the status message was one string rendered by both: saving Enabled features lit up the 2FA card in Security too — and with Features' wording, which talks about nav changes and says nothing true about the exemption. The message now carries the card that produced it, and each card gets text about what it actually changed: the exemption applies to the next sign-in. Covered by a component test that fails when the message is unscoped again.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes Settings page save feedback so success/error messages stay attached to the card/tab that initiated the save, avoiding misleading cross-card messaging when both cards persist through the same setSettings endpoint.
Changes:
- Scope the save status message to the originating Settings tab/card and customize the success text per card.
- Add a happy-dom component test covering scoped messaging across the Features and Security tabs.
- Update committed
web/distartifacts for the frontend rebuild.
Reviewed changes
Copilot reviewed 2 out of 5 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| web/src/pages/Settings.tsx | Scopes save feedback by tab/card and customizes success copy per card. |
| web/src/pages/Settings.dom.test.tsx | Adds DOM-level regression tests for scoped save feedback across tabs. |
| web/dist/index.html | Updates asset hash reference after rebuild. |
| web/dist/assets/CodeEditor-DS5r_yrM.js | Rebuilt bundle chunk reflecting updated asset linkage. |
Suppressed comments (4)
web/src/pages/Settings.tsx:44
save()sets an error message on failure, but the message currently has no success/failure flag. Oncemsgincludes anokboolean, populate it here so the renderer can style failures appropriately.
try {
await api.setSettings({ disabledSections: [...disabled], localhostNo2fa: no2fa });
setMsg({ scope, text: okText });
} catch {
setMsg({ scope, text: "Save failed" });
web/src/pages/Settings.tsx:86
- Once
msg.okis available, use it to picktext-okvstext-dangerso errors don't render with success styling.
{msg?.scope === "features" && <span className="text-sm text-ok">{msg.text}</span>}
web/src/pages/Settings.tsx:109
- Same as the Features card: when save fails, the status text should render with an error class rather than
text-ok.
{msg?.scope === "security" && <span className="text-sm text-ok">{msg.text}</span>}
web/src/pages/Settings.dom.test.tsx:78
- Use the
clickSave()helper (awaiting the mockedsetSettingspromise) instead ofact(async () => saveButton().click())so assertions run after the post-save status message has been applied.
it("shows the result on the card that saved", async () => {
await act(async () => saveButton().click());
expect(container.textContent).toContain("nav changes");
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+21
to
26
| // Scoped to the card that saved. Both cards write through the same endpoint | ||
| // (one settings payload), so a single message string meant pressing Save under | ||
| // Features also lit up the 2FA card in Security — with Features' wording, which | ||
| // is about nav changes and says nothing true about the exemption. | ||
| const [msg, setMsg] = useState<{ scope: Tab; text: string } | null>(null); | ||
| const [tab, setTab] = useState<Tab>("features"); |
Comment on lines
+68
to
+73
| function saveButton(): HTMLElement { | ||
| const el = [...container.querySelectorAll("button")].find((b) => b.textContent?.includes("Save settings")); | ||
| if (!el) throw new Error("save button not found"); | ||
| return el; | ||
| } | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reported from use: saving Enabled features also showed a success message on
Security → localhost 2FA exemption.
Both cards persist through the same endpoint (one settings payload), so the page
kept a single
msgstring and both cards rendered it. Two things were wrong withthat: the message appeared where nothing had been saved from, and the wording —
"Users may need to reload for nav changes to apply" — is about the feature flags
and says nothing true about the 2FA exemption.
The message now carries the card that produced it, and each card gets text about
what it actually changed (the exemption applies to the next sign-in).
Deliberately unchanged: Save still submits both values, because that is what
the endpoint takes. Splitting the payload would be a behaviour change beyond the
reported bug, and the current arrangement is harmless — it persists what's on
screen either way.
Type of change
Checklist
go test -short ./...andgo vet ./...pass — N/A (no Go changed)gofmtgate is clean — N/A (no Go changed)cd web && npx tsc --noEmit)web/distdocs/and added aCHANGELOG.mdentry — N/A: a status message on an admin page, no documented behaviour changesNotes for reviewers
The component test is the first thing the happy-dom environment from #130 has been
useful for outside its own wiring: this bug is invisible to a unit test and obvious
to a rendered one.
Mutation-tested — putting the shared message back makes two of the three fail
(
does not carry the Features message over to Security,gives Security its own wording). The third asserts the message still appears on the card that saved, so"render nothing" cannot pass either.