Skip to content

Commit 694a5df

Browse files
melvincarvalhomelvincarvalho
committed
review: address Copilot pickups on JavaScriptSolidServer#436
Three concrete fixes from the review thread. 1. .btn { display: inline-block } overrode the UA stylesheet's [hidden] { display: none } (equal specificity, author rule wins because it's later in the cascade). Sign up / Sign in would have flashed visible before the HEAD probe finished. Add an explicit [hidden] { display: none !important } so the attribute stays authoritative; new test pins the rule down. 2. The mode pill and feature-pills row were rendered into the seeded HTML at first start. Skip-if-exists then froze them — a mode change later would have left the seeded copy showing the old mode and feature flags even though the rest of the page is meant to adapt without regeneration. Drop both from the seeded template; the CLI banner already lists them at startup. The renderer drops the listFeatures helper and the singleUser/enabled context inputs. 3. The HEAD-decision matrix was only verified by regex checks on the inline script's text — a wrong-button regression for 200/403/404 would have slipped through. Extract the matrix into a pure helper, `decideRevealForRegisterStatus(status)`, and unit-test the five cases. The inline script implements the same matrix literally; the regex test still pins the literals so they don't drift silently. 819/819 tests pass.
1 parent 692932f commit 694a5df

3 files changed

Lines changed: 124 additions & 77 deletions

File tree

‎src/ui/server-root.html‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,12 @@
3535
cursor: pointer;
3636
transition: background 0.1s, border-color 0.1s;
3737
}
38+
/* The .btn { display: inline-block } above otherwise overrides the
39+
browser's UA `[hidden] { display: none }` rule (equal specificity,
40+
author rule wins because it's later in the cascade). Without this,
41+
the Sign up / Sign in buttons would flash visible before the HEAD
42+
probe runs. !important keeps the [hidden] attribute authoritative. */
43+
[hidden] { display: none !important; }
3844
.btn-primary { background: #2c2c2c; color: #fff; }
3945
.btn-primary:hover { background: #000; }
4046
.btn-secondary { background: #fff; color: #2c2c2c; border-color: #ccc; }
@@ -43,8 +49,6 @@
4349
.info .row { display: flex; justify-content: space-between; padding: 0.2rem 0; }
4450
.info .label { color: #999; }
4551
.info code { font-family: "SFMono-Regular", Consolas, "Liberation Mono", Menlo, monospace; font-size: 0.9em; color: #555; }
46-
.features { font-size: 0.85rem; color: #666; margin-top: 0.5rem; }
47-
.features span { display: inline-block; background: #eee; padding: 0.15rem 0.5rem; border-radius: 3px; margin-right: 0.3rem; margin-bottom: 0.3rem; font-family: "SFMono-Regular", Consolas, "Liberation Mono", Menlo, monospace; font-size: 0.8rem; }
4852
footer { margin-top: 2rem; padding-top: 1rem; border-top: 1px solid #ddd; color: #999; font-size: 0.8rem; text-align: center; }
4953
footer a { color: #888; }
5054
footer small { display: block; margin-top: 0.5rem; }
@@ -65,9 +69,12 @@ <h1>Welcome</h1>
6569

6670
<div class="info">
6771
<div class="row"><span class="label">Version</span><code>{{version}}</code></div>
68-
<div class="row"><span class="label">Mode</span><code>{{mode}}</code></div>
69-
<div class="features">{{features}}</div>
7072
</div>
73+
<!-- Mode and enabled-feature pills used to live here, but the seeded
74+
HTML can't reflect mode changes after first start (skip-if-exists).
75+
The CLI banner already lists both at startup; surface them via a
76+
runtime probe in a follow-up if needed. See #436 review. -->
77+
7178

7279
<footer>
7380
Powered by <a href="https://jss.live">JSS</a> · <a href="https://github.com/JavaScriptSolidServer/JavaScriptSolidServer">GitHub</a>

‎src/ui/server-root.js‎

Lines changed: 38 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -17,65 +17,62 @@ import { generatePublicReadAcl, serializeAcl } from '../wac/parser.js';
1717
const __dirname = dirname(fileURLToPath(import.meta.url));
1818
const TEMPLATE_PATH = join(__dirname, 'server-root.html');
1919

20-
/**
21-
* Collect the list of enabled features for display on the landing page.
22-
*/
23-
function listFeatures(options = {}) {
24-
const f = [];
25-
if (options.idp) f.push('idp');
26-
if (options.nostr) f.push('nostr');
27-
if (options.webrtc) f.push('webrtc');
28-
if (options.activitypub) f.push('activitypub');
29-
if (options.git) f.push('git');
30-
if (options.pay) f.push('payments');
31-
if (options.notifications) f.push('notifications');
32-
if (options.mashlib) f.push('mashlib');
33-
if (options.mongo) f.push('mongo');
34-
if (options.tunnel) f.push('tunnel');
35-
if (options.terminal) f.push('terminal');
36-
return f;
37-
}
38-
3920
/**
4021
* Render the landing page as an HTML string.
4122
*
42-
* The page itself is mode-agnostic — it doesn't change based on
43-
* single-user vs multi-user, and Sign up / Sign in are revealed at
44-
* load time by an inline HEAD probe against /idp/register. So the
45-
* same seeded HTML keeps working when the operator changes modes
46-
* without regenerating the file. See #435.
23+
* The page is mode-agnostic — same HTML for single-user and multi-user.
24+
* Sign up / Sign in are revealed at load time by an inline HEAD probe
25+
* against /idp/register, so the seeded file keeps working across mode
26+
* changes without regeneration. See #435.
27+
*
28+
* Only `version` is rendered into the seeded HTML — anything else that
29+
* varies with server state (mode, enabled features) would go stale on
30+
* the next mode change because of skip-if-exists.
4731
*
4832
* @param {object} ctx
49-
* @param {string} [ctx.version] - JSS version (rendered into the info box)
50-
* @param {boolean} [ctx.singleUser] - Drives the "Mode" label only
51-
* @param {object} [ctx.enabled] - Map of feature flags for the pills row
33+
* @param {string} [ctx.version] - JSS version (shown in the info box)
5234
* @returns {string} HTML
5335
*/
5436
export function renderServerRoot(ctx = {}) {
55-
const { version = 'unknown', singleUser = false, enabled = {} } = ctx;
56-
37+
const { version = 'unknown' } = ctx;
5738
const tpl = readFileSync(TEMPLATE_PATH, 'utf8');
58-
const mode = singleUser ? 'single-user' : 'multi-user';
59-
const features = listFeatures(enabled)
60-
.map(f => `<span>${f}</span>`)
61-
.join(' ');
6239

63-
// Single-pass token substitution. Each {{token}} in the original
64-
// template is matched once and replaced from `values`; substituted
65-
// text is not re-scanned, so a `$` or stray `{{…}}` in a value
66-
// can't cause re-substitution or hit String.prototype.replace's
67-
// `$&` substitution patterns. See #433 review thread.
40+
// Single-pass token substitution. Each {{token}} is matched once
41+
// against the original template and replaced from `values`;
42+
// substituted text isn't re-scanned (a `$` or stray `{{…}}` in a
43+
// value can't cause re-substitution or hit String.prototype.replace's
44+
// `$&` substitution patterns). See #433 review thread.
6845
const values = {
6946
title: 'JSS Solid pod',
70-
version: escape(version),
71-
mode,
72-
features
47+
version: escape(version)
7348
};
7449
return tpl.replace(/{{(\w+)}}/g, (match, key) =>
7550
Object.prototype.hasOwnProperty.call(values, key) ? values[key] : match
7651
);
7752
}
7853

54+
/**
55+
* Decide which conditional buttons (Sign up, Sign in) to reveal based
56+
* on the response status of `HEAD /idp/register`. Pure function so the
57+
* 200 / 403 / 404 matrix can be unit-tested without DOM. The inline
58+
* script in server-root.html implements the same matrix literally;
59+
* keep them in sync.
60+
*
61+
* 200 → registration open: reveal both Sign up and Sign in
62+
* 403 → IDP enabled but registration disabled (single-user mode):
63+
* reveal Sign in only
64+
* anything else (404, network error) → reveal neither (no IDP)
65+
*
66+
* @param {number|undefined} status - HTTP status code, or undefined for
67+
* network error.
68+
* @returns {{ register: boolean, login: boolean }}
69+
*/
70+
export function decideRevealForRegisterStatus(status) {
71+
if (status === 200) return { register: true, login: true };
72+
if (status === 403) return { register: false, login: true };
73+
return { register: false, login: false };
74+
}
75+
7976
function escape(s = '') {
8077
return String(s)
8178
.replace(/&/g, '&amp;')

‎test/server-root.test.js‎

Lines changed: 75 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import { describe, it, before, after } from 'node:test';
1212
import assert from 'node:assert';
1313
import fs from 'fs-extra';
1414
import { createServer } from '../src/server.js';
15-
import { renderServerRoot } from '../src/ui/server-root.js';
15+
import { renderServerRoot, decideRevealForRegisterStatus } from '../src/ui/server-root.js';
1616
import { startTestServer, stopTestServer, request, assertStatus } from './helpers.js';
1717

1818
describe('Server-root landing page', () => {
@@ -105,22 +105,29 @@ describe('Server-root landing — operator override', () => {
105105

106106
describe('renderServerRoot', () => {
107107
// Mode-agnostic copy: the same page is served regardless of single-user
108-
// vs multi-user. The status pill carries the mode label; the buttons
109-
// adapt at load time via the HEAD probe (verified separately below).
110-
it('renders the same mode-agnostic copy regardless of singleUser flag', () => {
111-
const single = renderServerRoot({ version: '1.0.0', singleUser: true });
112-
const multi = renderServerRoot({ version: '1.0.0', singleUser: false });
113-
114-
// Same welcome copy, same primary CTA, same explainer.
115-
for (const html of [single, multi]) {
116-
assert.match(html, /<h1>Welcome<\/h1>/);
117-
assert.match(html, /Your JSS Solid pod is running/);
118-
assert.match(html, /open standard for personal data/);
119-
}
120-
121-
// Mode pill differs.
122-
assert.match(single, /<code>single-user<\/code>/);
123-
assert.match(multi, /<code>multi-user<\/code>/);
108+
// vs multi-user. There's no mode pill or features list in the seeded
109+
// HTML — those would go stale on the next mode change because of
110+
// skip-if-exists. Mode/feature differences land in the buttons, which
111+
// adapt at load time via the HEAD probe (covered below).
112+
it('renders the same copy regardless of any context flags', () => {
113+
const a = renderServerRoot({ version: '1.0.0', singleUser: true });
114+
const b = renderServerRoot({ version: '1.0.0', singleUser: false });
115+
// Drop the only varying value (the version, identical here) and
116+
// assert byte-equality across the two renders.
117+
assert.strictEqual(a, b);
118+
assert.match(a, /<h1>Welcome<\/h1>/);
119+
assert.match(a, /Your JSS Solid pod is running/);
120+
assert.match(a, /open standard for personal data/);
121+
});
122+
123+
it('does not bake mode or feature pills into the seeded HTML', () => {
124+
// These would go stale: the seed is skip-if-exists, so a mode
125+
// change after first start wouldn't re-render them. Excluded
126+
// from the seed; the CLI banner already lists them at startup.
127+
const html = renderServerRoot({ version: '1.0.0', singleUser: true, enabled: { idp: true, nostr: true } });
128+
assert.doesNotMatch(html, /<code>single-user<\/code>/);
129+
assert.doesNotMatch(html, /<span>idp<\/span>/);
130+
assert.doesNotMatch(html, /<span>nostr<\/span>/);
124131
});
125132

126133
it('always emits the Get started button pointing at the docs introduction', () => {
@@ -134,20 +141,25 @@ describe('renderServerRoot', () => {
134141

135142
it('emits Sign up + Sign in buttons hidden for the HEAD probe to reveal', () => {
136143
const html = renderServerRoot({ version: '1.0.0' });
137-
// Both anchors are present in every mode; the inline script reveals
138-
// them based on what /idp/register actually returns.
139144
assert.match(html, /<a href="\/idp\/register"[^>]*data-cond="register"[^>]*hidden/);
140145
assert.match(html, /<a href="\/idp"[^>]*data-cond="login"[^>]*hidden/);
141146
assert.match(html, /Sign up/);
142147
assert.match(html, /Sign in/);
143148
});
144149

150+
it('overrides the .btn display rule for the [hidden] attribute so the buttons actually start hidden', () => {
151+
// Without an explicit !important [hidden] rule, the .btn class's
152+
// display:inline-block beats the UA stylesheet's [hidden]{display:none}
153+
// and the Sign up / Sign in anchors flash visible before the HEAD probe
154+
// finishes. The CSS rule is the load-bearing piece; assert it's there.
155+
const html = renderServerRoot({ version: '1.0.0' });
156+
assert.match(html, /\[hidden\]\s*\{[^}]*display:\s*none\s*!important/);
157+
});
158+
145159
it('includes the HEAD-adaptive script targeting /idp/register', () => {
146160
const html = renderServerRoot({ version: '1.0.0' });
147161
assert.match(html, /fetch\(['"]\/idp\/register['"]/);
148162
assert.match(html, /method:\s*['"]HEAD['"]/);
149-
// The three documented branches: 200 → both, 403 → login only,
150-
// anything else → neither. Assert the magic numbers are present.
151163
assert.match(html, /res\.status === 200/);
152164
assert.match(html, /res\.status === 403/);
153165
});
@@ -158,17 +170,6 @@ describe('renderServerRoot', () => {
158170
assert.match(html, /window\.location\.origin/);
159171
});
160172

161-
it('lists enabled features as pills', () => {
162-
const html = renderServerRoot({
163-
version: '1.0.0',
164-
enabled: { idp: true, nostr: true, webrtc: true, terminal: true }
165-
});
166-
assert.match(html, /<span>idp<\/span>/);
167-
assert.match(html, /<span>nostr<\/span>/);
168-
assert.match(html, /<span>webrtc<\/span>/);
169-
assert.match(html, /<span>terminal<\/span>/);
170-
});
171-
172173
it('interpolates version into the info box', () => {
173174
const html = renderServerRoot({ version: '9.9.9' });
174175
assert.match(html, /<code>9\.9\.9<\/code>/);
@@ -187,3 +188,45 @@ describe('renderServerRoot', () => {
187188
assert.match(html, /<code>\/index\.html<\/code>/);
188189
});
189190
});
191+
192+
// Pure-function unit tests for the HEAD response → button-reveal matrix.
193+
// The inline script in server-root.html implements the same matrix by
194+
// hand; a regex check on the script text (above) catches outright drops
195+
// of the literals, but only this helper test pins down the *behaviour*
196+
// of the matrix without needing a DOM.
197+
describe('decideRevealForRegisterStatus', () => {
198+
it('reveals both Sign up and Sign in for HTTP 200 (registration open)', () => {
199+
assert.deepStrictEqual(
200+
decideRevealForRegisterStatus(200),
201+
{ register: true, login: true }
202+
);
203+
});
204+
205+
it('reveals only Sign in for HTTP 403 (single-user — registration disabled)', () => {
206+
assert.deepStrictEqual(
207+
decideRevealForRegisterStatus(403),
208+
{ register: false, login: true }
209+
);
210+
});
211+
212+
it('reveals neither for HTTP 404 (no IDP)', () => {
213+
assert.deepStrictEqual(
214+
decideRevealForRegisterStatus(404),
215+
{ register: false, login: false }
216+
);
217+
});
218+
219+
it('reveals neither for any other status (e.g. 500)', () => {
220+
assert.deepStrictEqual(
221+
decideRevealForRegisterStatus(500),
222+
{ register: false, login: false }
223+
);
224+
});
225+
226+
it('reveals neither when status is undefined (network error)', () => {
227+
assert.deepStrictEqual(
228+
decideRevealForRegisterStatus(undefined),
229+
{ register: false, login: false }
230+
);
231+
});
232+
});

0 commit comments

Comments
 (0)