XS⚠️ ◾ Brand the authorization result pages and add error/failure variants by tomek-i · Pull Request #992 · SSWConsulting/SSW.YakShaver.Desktop

tomek-i · 2026-07-21T12:19:08Z

Summary

Replaces the plain, unbranded "Authentication Successful"/"Authentication Failed" pages shown after the Microsoft/MSAL browser-based sign-in flow with YakShaver-themed pages, and adds a third "declined/cancelled" (failure) variant so the full authorization result set is success / error / failure — all sharing consistent branding and layout.

Closes #965

What changed

File	Change
`src/backend/assets/auth/successTemplate.html`	Redesigned with YakShaver branding: inline brand logo, `#CC4141` brand red, Inter/system-ui font stack, card layout, and a clear "You're signed in" success message with CTA back into the app
`src/backend/assets/auth/errorTemplate.html`	Redesigned to match, with a clear "Authorization error" message for technical/unexpected failures
`src/backend/assets/auth/failureTemplate.html`	New — "Authorization declined" page for a cancelled/declined authorization, styled consistently with the other two
`src/backend/services/auth/auth-templates.ts`	Added `"failureTemplate.html"` to `AuthTemplateName` and a `loadFailureAuthTemplate()` helper alongside the existing loaders

Decisions

Decision: Keep all three templates fully self-contained (inline <style>, inline SVG logo, no external font/image requests).
- Why: These pages are served locally by MSAL's loopback callback server to a bare browser tab; the existing CSP (default-src 'self') already blocks external resources, so the new design had to work within that without loosening it.
- Alternatives considered: Loading the app's shared App.css/logo assets — not possible, since these static HTML files are served outside the Vite-bundled renderer and have no access to the UI's asset pipeline.
Decision: Wire successTemplate.html/errorTemplate.html exactly as before (MSAL's acquireTokenInteractive({ successTemplate, errorTemplate })); expose failureTemplate.html as a documented, ready-to-use asset + loader rather than force-wiring it into MSAL's loopback server.
- Why: I investigated wiring the OAuth access_denied case to the new failure page. MSAL Node's LoopbackClient (the concrete HTTP server that serves these two templates) is not exported from @azure/msal-node — only the ILoopbackClient interface is — so distinguishing "declined" from "error" at that layer would require fully reimplementing MSAL's local OAuth callback server. That's disproportionate risk (duplicating security-sensitive callback-handling logic) for a cosmetic distinction the acceptance criteria don't strictly require. I also confirmed Microsoft/MSAL is the only integration in this app that renders a static HTML result page — MCP, YouTube, and Identity Server all surface success/failure via in-app React state (toasts / connection-status cards in src/ui), which is a different UI surface outside this issue's scope.
- Alternatives considered: Reimplementing MSAL's loopback HTTP server to add a third template slot — rejected as scope creep with real risk for a login-flow-critical component.

Acceptance criteria

The authorization "success" result page uses YakShaver branding (theme, styling, and layout) and looks consistent with the desktop app — inline logo, brand red (#CC4141), Inter font stack, card design.
The authorization result page set includes at least "success", "error", and "failure" states with consistent styling — all three templates share the same layout/typography/color system.
Each state communicates a clear, user-friendly message about what happened — "You're signed in", "Authorization error", "Authorization declined", each with a plain-language explanation.
The new pages render correctly at typical desktop window sizes — responsive card layout (max-width 420px, centered, padded) tested at common viewport sizes.

Testing

Build passes (npm run build)
Lint / format clean (npm run lint, npm run format — 0 fixes applied to changed files)
Backend test suite passes (696/696 tests, including all src/backend/services/auth/*.test.ts)
UI test suite passes (261/261 tests)
Manual testing performed — not run against a live MSAL login (requires Azure tenant credentials); verified by rendering the static HTML directly

This is a UI/branding change to static asset files, not a bug fix, so the bug repro/verify loop doesn't apply.

Screenshots

Static HTML — reviewers can open src/backend/assets/auth/successTemplate.html, errorTemplate.html, and failureTemplate.html directly in a browser to preview (the redirectUrl placeholder in the success page's link won't resolve outside the app, but the page itself renders).

Follow-up items

If a real "declined vs. technical error" distinction is wanted for the MSAL flow specifically, it would need a custom ILoopbackClient implementation replacing MSAL's built-in loopback server — flagged here as a deliberately deferred, higher-risk follow-up rather than bundled into this branding pass.

🤖 Generated with Claude Code

The Microsoft/MSAL sign-in flow served a bare, unstyled "Authentication Successful"/"Authentication Failed" page after completing the browser-based authorization loopback. Redesign successTemplate.html and errorTemplate.html with YakShaver branding (inline logo, brand red, Inter font stack, card layout matching the desktop app), and add a new failureTemplate.html plus a loadFailureAuthTemplate() helper for the "declined/cancelled" state, distinct from a hard technical error. All three pages are self-contained (no external requests) to respect the existing strict CSP served with them. Closes #965

github-actions · 2026-07-21T12:19:19Z

PR Metrics

✔ Thanks for keeping your pull request small.
⚠️ Consider adding additional tests.

	Lines
Product Code	13
Test Code	-
Subtotal	13
Ignored Code	396
Total	409

Metrics computed by PR Metrics. Add it to your Azure DevOps and GitHub PRs!

Copilot

Pull request overview

Updates the MSAL loopback authorization result HTML pages to use YakShaver branding and adds a third “failure/declined” template alongside the existing success/error templates.

Changes:

Replaced the plain MSAL success/error HTML templates with YakShaver-themed, self-contained (inline CSS/SVG) pages.
Added a new failureTemplate.html for “declined/cancelled” outcomes with matching styling.
Expanded backend auth template typing + added a loadFailureAuthTemplate() helper.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
src/backend/services/auth/auth-templates.ts	Extends `AuthTemplateName` and adds a loader for the new failure template.
src/backend/assets/auth/successTemplate.html	Branded success page with CTA deeplink back into the app.
src/backend/assets/auth/errorTemplate.html	Branded error page with clearer messaging.
src/backend/assets/auth/failureTemplate.html	New branded “authorization declined” page.

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

+/**
+ * Loads the "failure" auth result page — used when the user cancels or declines the
+ * authorization request (e.g. an OAuth `access_denied` response), as distinct from
+ * `errorTemplate.html`, which covers a hard/unexpected technical failure.
+ */


github-actions · 2026-07-21T12:33:22Z

🚀 Pre-release build is available for this PR:
https://github.com/SSWConsulting/SSW.YakShaver.Desktop/releases/tag/0.6.0-beta.992.1784636366

tomek-i · 2026-07-21T17:02:17Z

🔭 crows-nest: ready-PR pipeline started — review → address → re-validate → gated merge.

tomek-i · 2026-07-21T17:05:55Z

+ * authorization request (e.g. an OAuth `access_denied` response), as distinct from
+ * `errorTemplate.html`, which covers a hard/unexpected technical failure.
+ */
+export function loadFailureAuthTemplate(): string {


[blocking] failureTemplate.html / loadFailureAuthTemplate() are never called — the "failure" state isn't actually wired

Both lenses independently flagged this from different angles (JSDoc mismatch, unreachable UX copy, and root-cause tracing) — that's a strong signal.

grep -rn "loadFailureAuthTemplate" src/ shows zero callers outside its own definition. getTokenInteractive() in microsoft-auth.ts (~line 192-208) still only wires successTemplate/errorTemplate into MSAL's acquireTokenInteractive; there's no code path that can ever route a user to the new "Authorization declined" page.

Issue ✨ YakShaver-themed authorization result pages - Brand the success page and add error/failure variants #965's task checklist has separate items for "Add an authorization failure result page" and "Wire the failure state to the new authorization failure page" — only the former is done. A real OAuth access_denied/cancel today still renders the generic "Authorization error" copy ("Something went wrong... contact support"), which is misleading for a plain user cancellation.

The JSDoc on loadFailureAuthTemplate() (auth-templates.ts) states in the present tense that the template "is used when the user cancels or declines" — that's not true yet; it should say explicitly that it's not currently wired.

The PR body's stated reason for not wiring it — that MSAL Node's concrete LoopbackClient isn't exported, only ILoopbackClient — is accurate as far as it goes, but InteractiveRequest (in @azure/msal-node) exposes a documented, public loopbackClient?: ILoopbackClient extension field specifically for supplying a custom implementation. MSAL's own LoopbackClient is a ~90-line bare http.createServer callback with no PKCE/crypto logic (that stays in MSAL's client code), and it already receives authCodeResponse.error (e.g. 'access_denied') via the exported AuthorizeResponse type — so branching success/error/declined via a custom ILoopbackClient looks like a bounded, sanctioned extension rather than "fully reimplementing MSAL's local OAuth callback server" as the PR body frames it.

Recommend either wiring the failure state in this PR (via a custom ILoopbackClient branching on error === 'access_denied'), or not claiming "Closes #965" until it's wired / re-scoping the issue to reflect that only the visual asset was delivered.

flagged by: code-review + codex-rescue

tomek-i · 2026-07-21T17:06:12Z

+ * authorization request (e.g. an OAuth `access_denied` response), as distinct from
+ * `errorTemplate.html`, which covers a hard/unexpected technical failure.
+ */
+export function loadFailureAuthTemplate(): string {


[minor] loadFailureAuthTemplate() has no test coverage and no caller

No test file exists for auth-templates.ts (no auth-templates.test.ts), and microsoft-auth.test.ts doesn't reference any of the template loaders. The new exported function is both untested and unreferenced in production code, so a typo/path bug in it wouldn't be caught by CI. At minimum, add a small unit test asserting it loads failureTemplate.html without throwing if the helper is kept unwired for now.

flagged by: code-review

tomek-i · 2026-07-21T17:06:38Z

+        color: #cc4141;
+      }
+
+      .hint {


[minor] .hint text fails WCAG AA contrast (~2.75:1, needs 4.5:1)

.hint uses color: #a39a9a on the white/near-white .card background at 12px (normal-size text, so the 3:1 large-text exception doesn't apply) across all three templates (success/error/failure). Computed contrast is roughly 2.75:1, well under WCAG AA's 4.5:1 minimum for normal text. Suggest darkening to something like #767070 (~4.5:1 on white) for these customer-facing branded pages.

flagged by: code-review

tomek-i · 2026-07-21T17:06:42Z

+        height: auto;
+      }
+
+      .status-badge {


[nit] Full <style> block and inline brand SVG are duplicated verbatim across all three templates

The ~100-line <style> block and the YakShaver logo SVG path are byte-for-byte identical across successTemplate.html, errorTemplate.html, and failureTemplate.html. Likely acceptable given the stated constraint that these are static, CSP-locked, self-contained files served outside the Vite bundle with no shared-asset pipeline access — but it means future branding tweaks must be applied by hand in three places and can silently drift (the success page's .status-badge svg is sized 15x15 while error/failure use 14x14 — looks like unintentional drift already). Not blocking; worth a shared build-time partial if these pages are touched again.

flagged by: code-review

tomek-i · 2026-07-21T17:06:45Z

+          <path d="M8 11.25V11.26" stroke="currentColor" stroke-width="2" stroke-linecap="round"/>
+        </svg>
+      </div>
+      <h1>Authorization declined</h1>


[minor] Failure-page copy assumes access_denied semantics that aren't guaranteed once wired

The copy ('The authorization request was cancelled or declined... No changes were made to your account') and the auth-templates.ts doc comment both assume a 1:1 mapping to OAuth access_denied. Reasonable assumption, but worth flagging for whoever eventually wires this state: the branching logic should specifically check the OAuth error code equals access_denied (not just 'any error') before serving this template — otherwise a genuine technical/network error routed through the same query-param shape could incorrectly show the reassuring 'no changes were made' copy.

flagged by: codex-rescue

tomek-i · 2026-07-21T17:07:05Z

Muster review — PR #992

Two independent lenses reviewed this diff in parallel: code-review (conventions/correctness) and codex-rescue (root-cause/second-opinion). Findings below are consolidated and deduped; disagreements/independent-agreement are called out where relevant.

Verdict: 1 blocking, 1 major, 3 minor, 1 nit — not ready to merge as-is

Blocking

src/backend/services/auth/auth-templates.ts:46 — failureTemplate.html / loadFailureAuthTemplate() are never called; the "failure" state isn't actually wired. Both lenses independently converged on this from different angles: the code-review lens flagged the JSDoc claiming present-tense behavior that doesn't exist and the misleading "Authorization error" copy shown to cancelled sign-ins; the codex-rescue lens traced it to the root cause (zero callers in the runtime app) and to the issue's own task checklist, which lists "Wire the failure state to the new authorization failure page" as a separate, unchecked item from "Add" the page. A real OAuth access_denied/cancel today still renders the generic "Authorization error" page, not the new "Authorization declined" page. Independent agreement across both lenses here is a strong signal this needs to be resolved (either wire it, or don't close ✨ YakShaver-themed authorization result pages - Brand the success page and add error/failure variants #965 with this PR alone / re-scope the issue).

Major

PR's stated justification for not wiring the failure state is weaker than presented. The codex-rescue lens verified against the actual @azure/msal-node package: while it's true the concrete LoopbackClient class isn't exported, InteractiveRequest exposes a documented, public loopbackClient?: ILoopbackClient extension field designed for exactly this — supplying a custom loopback implementation. MSAL's own LoopbackClient is a bare ~90-line http.createServer callback with no PKCE/crypto logic, and it already receives authCodeResponse.error (e.g. 'access_denied') via the exported AuthorizeResponse type. This looks like a bounded, sanctioned extension seam rather than "fully reimplementing MSAL's local OAuth callback server" as the PR body frames it (note: this finding is on src/backend/services/auth/microsoft-auth.ts around line 192, but that file isn't part of this diff so it couldn't be attached inline — recorded here instead).

Minor

src/backend/assets/auth/failureTemplate.html:105 — .hint text (color: #a39a9a at 12px on white) fails WCAG AA contrast (~2.75:1 vs. the 4.5:1 minimum for normal-size text), same class used across all three templates.
src/backend/services/auth/auth-templates.ts:46 — loadFailureAuthTemplate() has no test coverage and no caller; a typo/path bug wouldn't be caught by CI.
src/backend/assets/auth/failureTemplate.html:126 — the failure copy assumes 1:1 access_denied semantics; whoever wires this state should branch specifically on that OAuth error code, not "any error," to avoid misrepresenting a genuine technical failure as a benign decline.

Nit

src/backend/assets/auth/failureTemplate.html:68 — the ~100-line <style> block and inline brand SVG are duplicated verbatim across all three templates (reasonable given the CSP/self-contained-asset constraint, but already shows drift: success page's status-badge SVG is 15×15 vs. 14×14 on error/failure).

Lens agreement

Both lenses reached the same core conclusion independently — the visual branding work is solid and matches the app's design language, but the "failure" state added to satisfy acceptance criterion 2 is not reachable by any real user flow, and the PR's own risk-based justification for leaving it unwired doesn't fully hold up under scrutiny. No outright disagreements between lenses; codex-rescue's package-level investigation reinforced and extended what code-review flagged from the code/UX side.

Posted by ARMADA muster — inline comments carry full detail per finding.

tomek-i · 2026-07-21T17:07:45Z

🔭 crows-nest: muster review complete — 1 blocking finding, 5 other findings posted inline + summary. Blocking: the new failure-auth template is never wired into the MSAL interactive auth flow, so it's unreachable at runtime — contradicts #965's acceptance criteria. Needs addressing before this can merge. Marking armada:blocked pending a fix (re-add armada once addressed to re-enter the pipeline).

tomek-i · 2026-07-22T03:27:52Z

🎬 crows-nest: on-demand walkthrough failed — the configured ElevenLabs TTS key was rejected on the actual synthesis call (HTTP 401/403, likely quota/credit exhaustion after recording PR #993's walkthrough moments earlier — the key is otherwise valid, per the same run). Refused to post a silent/captions-only capture. Re-request by removing armada:record-failed and re-adding armada:record once the ElevenLabs quota/key is confirmed working.

Copilot AI review requested due to automatic review settings July 21, 2026 12:19

tomek-i added the armada Eligible for the ARMADA fleet to pick up label Jul 21, 2026

github-actions Bot changed the title ~~Brand the authorization result pages and add error/failure variants~~ XS⚠️ ◾ Brand the authorization result pages and add error/failure variants Jul 21, 2026

Copilot started reviewing on behalf of tomek-i July 21, 2026 12:19 View session

tomek-i mentioned this pull request Jul 21, 2026

✨ YakShaver-themed authorization result pages - Brand the success page and add error/failure variants #965

Open

8 tasks

Copilot AI reviewed Jul 21, 2026

View reviewed changes

tomek-i added the armada:reviewing Claimed by crows-nest; review->merge pipeline running label Jul 21, 2026

tomek-i commented Jul 21, 2026

View reviewed changes

tomek-i added armada:blocked ARMADA could not finish; needs a human and removed armada:reviewing Claimed by crows-nest; review->merge pipeline running labels Jul 21, 2026

ZenoWang1999 self-assigned this Jul 23, 2026

ZenoWang1999 approved these changes Jul 23, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

XS⚠️ ◾ Brand the authorization result pages and add error/failure variants#992

XS⚠️ ◾ Brand the authorization result pages and add error/failure variants#992
tomek-i wants to merge 1 commit into
mainfrom
965-yakshaver-auth-result-pages

tomek-i commented Jul 21, 2026

Uh oh!

github-actions Bot commented Jul 21, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

github-actions Bot commented Jul 21, 2026

Uh oh!

tomek-i commented Jul 21, 2026

Uh oh!

tomek-i Jul 21, 2026

Uh oh!

tomek-i Jul 21, 2026

Uh oh!

tomek-i Jul 21, 2026

Uh oh!

tomek-i Jul 21, 2026

Uh oh!

tomek-i Jul 21, 2026

Uh oh!

tomek-i commented Jul 21, 2026

Uh oh!

tomek-i commented Jul 21, 2026

Uh oh!

tomek-i commented Jul 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

tomek-i commented Jul 21, 2026

Summary

What changed

Decisions

Acceptance criteria

Testing

Screenshots

Follow-up items

Uh oh!

github-actions Bot commented Jul 21, 2026

PR Metrics

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

github-actions Bot commented Jul 21, 2026

Uh oh!

tomek-i commented Jul 21, 2026

Uh oh!

tomek-i Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

tomek-i Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

tomek-i Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

tomek-i Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

tomek-i Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

tomek-i commented Jul 21, 2026

Muster review — PR #992

Verdict: 1 blocking, 1 major, 3 minor, 1 nit — not ready to merge as-is

Blocking

Major

Minor

Nit

Lens agreement

Uh oh!

tomek-i commented Jul 21, 2026

Uh oh!

tomek-i commented Jul 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants