Skip to content

feat(settings): Discord section in the local settings page (#272) - #318

Merged
rowkav09 merged 4 commits into
mainfrom
feat/settings-discord
Sep 23, 2026
Merged

rowkav09 merged 4 commits into
mainfrom
feat/settings-discord

Conversation

@rowkav09

@rowkav09 rowkav09 commented Sep 23, 2026 •

Copy link
Copy Markdown
Member

Closes #272. Part of #253.

Adds /settings to the running app with a Discord section:

  • on/off, timer (time played and left, played, left, none) and album art lookup (MusicBrainz or server art only)
  • saves to config.json in one step (temp file + rename), checked by the same rules as setup
  • applies without a restart: the Discord loop stops and starts again with the new settings
  • plain layout on the status page's styles; state text uses the existing blue/orange classes, no red/green

Security: the app server now has a session secret. Saves (PUT /api/settings) need the SameSite=Strict cookie the app's own pages set, JSON content type, and a same-origin request; /api/settings also refuses cross-site fetches. Only the discord section with enabled, timestamps and artworkLookup is accepted; errors return a short code, never paths or values.

Config: discord.timestamps is new and optional. Left out means "both", so configs written by setup are unchanged. The Discord presence loop now takes the timer setting.

Status and Settings pages link to each other. Not in this PR: the other settings sections.

Tests: store, handler and an end-to-end run against the real app (cookie required, other origins refused, bad values rejected, Discord turned off live).

@github-actions

Copy link
Copy Markdown
Contributor

/mira pause

@github-actions github-actions Bot added the mira-paused Pause automatic Mira reviews on this pull request label Sep 23, 2026
@codecov

codecov Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.72881% with 3 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/app-settings.js 95.38% 3 Missing ⚠️

📢 Thoughts on this report? Let us know!

Comment thread test/settings-page-handler.test.js Fixed
@rowkav09

Copy link
Copy Markdown
Member Author

CodeQL is right about this one: test/settings-page-handler.test.js:18 reintroduces the case-sensitive <script> assertion regex that #311 just fixed in the sibling test files (assert.doesNotMatch(page.body, /<script>|\sstyle=|\son[a-z]+=/)). As written the check would pass even if the page served an uppercase <SCRIPT> tag, which is exactly the alert CodeQL is gating on.

Same fix as #311 - make it case-insensitive and cover attribute-bearing tags:

assert.doesNotMatch(page.body, /<script\b[^>]*>[^<]|\sstyle=|\son[a-z]+=/i);

@rowkav09
rowkav09 force-pushed the feat/settings-discord branch from c9002db to 1b3fbf8 Compare September 23, 2026 23:51
@rowkav09

Copy link
Copy Markdown
Member Author

Fixed in 45cc5af: the check now counts <script occurrences on the lowercased page instead of using a tag regex. CodeQL on the latest head is green. No Mira review here (auto-paused).

@rowkav09
rowkav09 merged commit 2d0b64d into main Sep 23, 2026
12 checks passed
@rowkav09
rowkav09 deleted the feat/settings-discord branch September 23, 2026 23:58
@github-project-automation github-project-automation Bot moved this from Backlog to Done in nowplaying Sep 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:server mira-paused Pause automatic Mira reviews on this pull request size:L

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Settings: Discord section in the web UI

2 participants