sneak/AutistMask
1
0
Fork 0
Code Issues 1 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

feat: show debug banner on testnet or debug mode, add TESTNET tag #143

Merged
sneak merged 3 commits from fix/issue-140-debug-banner into main 2026-03-01 21:55:36 +01:00
Conversation 7 Commits 3 Files Changed 2 +28 -8
clawbot commented 2026-03-01 20:18:02 +01:00
Collaborator
Copy Link

Display the red debug banner when on a testnet OR when DEBUG is enabled.

When on a testnet, a "TESTNET" label is shown on the far right side of the banner. The banner label shows the network name when not in debug mode, and "DEBUG / INSECURE" when debug is on.

closes #140

Display the red debug banner when on a testnet OR when DEBUG is enabled. When on a testnet, a "TESTNET" label is shown on the far right side of the banner. The banner label shows the network name when not in debug mode, and "DEBUG / INSECURE" when debug is on. closes #140
clawbot added 1 commit 2026-03-01 20:18:03 +01:00
feat: show debug banner on testnet or debug mode, add TESTNET tag
All checks were successful
check / check (push) Successful in 26s
Details
29ff5393d3 
Display the red debug banner when on a testnet OR when DEBUG is enabled.
When on a testnet, a 'TESTNET' label is shown on the far right side of
the banner. The banner label shows the network name when not in debug
mode, and 'DEBUG / INSECURE' when debug is on.

closes #140
clawbot added the
bot
needs-review
 labels 2026-03-01 20:18:16 +01:00
sneak commented 2026-03-01 20:19:37 +01:00
Owner
Copy Link

if debug+testnet just do banner.textContent = "DEBUG / INSECURE [TESTNET]"; and if just testnet no debug do "[TESTNET]". no need for a separate element.

if debug+testnet just do `banner.textContent = "DEBUG / INSECURE [TESTNET]";` and if just testnet no debug do "[TESTNET]". no need for a separate element.
clawbot commented 2026-03-01 20:20:47 +01:00
Author
Collaborator
Copy Link

[manager] Self-review detected — removing merge-ready. Dispatching an independent reviewer.

**[manager]** Self-review detected — removing `merge-ready`. Dispatching an independent reviewer.
clawbot commented 2026-03-01 20:23:08 +01:00
Author
Collaborator
Copy Link

Independent Review — FAIL

Reviewer: clawbot (independent review agent)

Summary

docker build . passes. The changed files are scoped correctly (only src/popup/index.js and src/popup/views/helpers.js). No test weakening, no config changes, no unrelated modifications.

However, this PR does not implement the rework feedback from sneak posted here.

Issue: sneak's feedback not addressed

sneak explicitly requested:

if debug+testnet just do banner.textContent = "DEBUG / INSECURE [TESTNET]"; and if just testnet no debug do "[TESTNET]". no need for a separate element.

The current implementation does the opposite:

  1. Creates multiple child <span> elements with flexbox layout and absolute positioning — sneak said "no need for a separate element"
  2. debug+testnet case: Shows "DEBUG / INSECURE" centered with "TESTNET" absolutely positioned on the right — sneak wants just "DEBUG / INSECURE [TESTNET]" as plain textContent
  3. testnet-only case: Shows the full network name (e.g., "Sepolia Testnet") centered with "TESTNET" on the right — sneak wants just "[TESTNET]"
  4. The showView() helper in helpers.js also uses the multi-element approach, appending (viewName) to the label span

What needs to change

  • Remove the separate <span> elements and flexbox/absolute-positioning CSS
  • Use simple banner.textContent assignment:
    • DEBUG && isTestnet → "DEBUG / INSECURE [TESTNET]"
    • DEBUG && !isTestnet → "DEBUG / INSECURE" (existing behavior)
    • !DEBUG && isTestnet → "[TESTNET]"
  • Same logic in showView() with the (viewName) suffix for debug mode

Marking as needs-rework.

## Independent Review — FAIL **Reviewer**: clawbot (independent review agent) ### Summary `docker build .` passes. The changed files are scoped correctly (only `src/popup/index.js` and `src/popup/views/helpers.js`). No test weakening, no config changes, no unrelated modifications. However, this PR does **not** implement the rework feedback from sneak posted [here](https://git.eeqj.de/sneak/AutistMask/pulls/143#issuecomment-1404). ### Issue: sneak's feedback not addressed sneak explicitly requested: > if debug+testnet just do `banner.textContent = "DEBUG / INSECURE [TESTNET]";` and if just testnet no debug do `"[TESTNET]"`. no need for a separate element. The current implementation does the opposite: 1. **Creates multiple child `<span>` elements** with flexbox layout and absolute positioning — sneak said "no need for a separate element" 2. **debug+testnet case**: Shows "DEBUG / INSECURE" centered with "TESTNET" absolutely positioned on the right — sneak wants just `"DEBUG / INSECURE [TESTNET]"` as plain textContent 3. **testnet-only case**: Shows the full network name (e.g., "Sepolia Testnet") centered with "TESTNET" on the right — sneak wants just `"[TESTNET]"` 4. The `showView()` helper in `helpers.js` also uses the multi-element approach, appending `(viewName)` to the label span ### What needs to change - Remove the separate `<span>` elements and flexbox/absolute-positioning CSS - Use simple `banner.textContent` assignment: - `DEBUG && isTestnet` → `"DEBUG / INSECURE [TESTNET]"` - `DEBUG && !isTestnet` → `"DEBUG / INSECURE"` (existing behavior) - `!DEBUG && isTestnet` → `"[TESTNET]"` - Same logic in `showView()` with the `(viewName)` suffix for debug mode Marking as **needs-rework**.
clawbot added
needs-rework
 and removed
needs-review
 labels 2026-03-01 20:23:16 +01:00
clawbot self-assigned this 2026-03-01 20:23:42 +01:00
clawbot force-pushed fix/issue-140-debug-banner from 29ff5393d3 to 2f69ad0361 2026-03-01 20:30:48 +01:00 Compare
clawbot commented 2026-03-01 20:30:59 +01:00
Author
Collaborator
Copy Link

Simplified the banner implementation per sneak's feedback:

  • Removed multi-<span> elements, flexbox, and absolute positioning
  • Banner now uses simple textContent assignment:
    • Debug + testnet: "DEBUG / INSECURE [TESTNET]"
    • Testnet only: "[TESTNET]"
    • Debug only: "DEBUG / INSECURE" (original behavior)
  • showView() updated to use the same approach on the banner element directly
  • docker build . passes (all 49 tests green, formatting clean)
Simplified the banner implementation per [sneak's feedback](https://git.eeqj.de/sneak/AutistMask/pulls/143#issuecomment-1404): - Removed multi-`<span>` elements, flexbox, and absolute positioning - Banner now uses simple `textContent` assignment: - Debug + testnet: `"DEBUG / INSECURE [TESTNET]"` - Testnet only: `"[TESTNET]"` - Debug only: `"DEBUG / INSECURE"` (original behavior) - `showView()` updated to use the same approach on the banner element directly - `docker build .` passes (all 49 tests green, formatting clean)
clawbot added
needs-review
 and removed
needs-rework
 labels 2026-03-01 20:31:01 +01:00
clawbot removed their assignment 2026-03-01 20:31:10 +01:00
clawbot commented 2026-03-01 20:34:21 +01:00
Author
Collaborator
Copy Link

Independent Review — FAIL

Reviewer: clawbot (independent review agent)

What's Good

  • docker build . passes (format, lint, tests, build all green)
  • Only 2 files changed (src/popup/index.js, src/popup/views/helpers.js) — properly scoped
  • No test weakening, no config changes, no unrelated modifications
  • The banner text content logic exactly matches sneak's instructions: simple textContent, no extra elements, no flexbox, no absolute positioning
  • All three text cases are present:
    • "DEBUG / INSECURE [TESTNET]" (debug + testnet)
    • "[TESTNET]" (testnet only)
    • "DEBUG / INSECURE" (debug only)

Issue: Testnet-only banner never appears in production builds

In init(), the banner creation happens before loadState():

async function init() {
    const net = currentNetwork();       // ← state.networkId is still "mainnet" (default)
    if (DEBUG || net.isTestnet) {        // ← net.isTestnet is always false here
        // ... create banner ...
    }

    await loadState();                   // ← actual persisted networkId loaded here
    // ...
    showView("main");                    // ← tries to update banner, but element doesn't exist
}

currentNetwork() reads state.networkId, which is "mainnet" (the DEFAULT_STATE value) at that point because loadState() hasn't run yet. So net.isTestnet is always false before loadState().

In DEBUG mode (current dev build where DEBUG = true): The banner is always created because DEBUG is true regardless, and showView() later corrects the text to include [TESTNET] after state is loaded. So it works, with a brief flash of wrong text.

In production builds (DEBUG = false): The testnet-only banner is never created — DEBUG is false and net.isTestnet is false (pre-loadState), so the if block is skipped entirely. After loadState(), showView() tries to find #debug-banner to update it, but the element doesn't exist.

The testnet-only case (one of the three required cases) is broken in production.

Fix

Move the banner creation to after loadState(), or create the banner element in showView() if it doesn't exist yet. This is a small change.

Verdict

FAIL — The text content logic is correct and matches sneak's instructions perfectly, but the load ordering bug means the testnet-only banner never appears in production builds. Needs a small rework to move banner creation after loadState().

## Independent Review — FAIL **Reviewer**: clawbot (independent review agent) ### What's Good - `docker build .` passes (format, lint, tests, build all green) - Only 2 files changed (`src/popup/index.js`, `src/popup/views/helpers.js`) — properly scoped - No test weakening, no config changes, no unrelated modifications - The banner text content logic exactly matches [sneak's instructions](https://git.eeqj.de/sneak/AutistMask/pulls/143#issuecomment-1404): simple `textContent`, no extra elements, no flexbox, no absolute positioning - All three text cases are present: - `"DEBUG / INSECURE [TESTNET]"` (debug + testnet) - `"[TESTNET]"` (testnet only) - `"DEBUG / INSECURE"` (debug only) ### Issue: Testnet-only banner never appears in production builds In `init()`, the banner creation happens **before** `loadState()`: ```javascript async function init() { const net = currentNetwork(); // ← state.networkId is still "mainnet" (default) if (DEBUG || net.isTestnet) { // ← net.isTestnet is always false here // ... create banner ... } await loadState(); // ← actual persisted networkId loaded here // ... showView("main"); // ← tries to update banner, but element doesn't exist } ``` `currentNetwork()` reads `state.networkId`, which is `"mainnet"` (the `DEFAULT_STATE` value) at that point because `loadState()` hasn't run yet. So `net.isTestnet` is always `false` before `loadState()`. **In DEBUG mode** (current dev build where `DEBUG = true`): The banner is always created because `DEBUG` is true regardless, and `showView()` later corrects the text to include `[TESTNET]` after state is loaded. So it works, with a brief flash of wrong text. **In production builds** (`DEBUG = false`): The testnet-only banner is **never created** — `DEBUG` is false and `net.isTestnet` is false (pre-loadState), so the `if` block is skipped entirely. After `loadState()`, `showView()` tries to find `#debug-banner` to update it, but the element doesn't exist. The testnet-only case (one of the three required cases) is broken in production. ### Fix Move the banner creation to after `loadState()`, or create the banner element in `showView()` if it doesn't exist yet. This is a small change. ### Verdict **FAIL** — The text content logic is correct and matches sneak's instructions perfectly, but the load ordering bug means the testnet-only banner never appears in production builds. Needs a small rework to move banner creation after `loadState()`.
clawbot added
needs-rework
 and removed
needs-review
 labels 2026-03-01 20:34:29 +01:00
clawbot self-assigned this 2026-03-01 20:34:58 +01:00
clawbot force-pushed fix/issue-140-debug-banner from 2f69ad0361 to d22c83e6dd 2026-03-01 20:36:24 +01:00 Compare
clawbot commented 2026-03-01 20:36:33 +01:00
Author
Collaborator
Copy Link

Rework complete — fixed banner load ordering bug.

Moved await loadState() and applyTheme() to before the banner creation block in init(). Previously, currentNetwork() was called before loadState(), so state.networkId was still the default "mainnet" — meaning net.isTestnet was always false and the testnet banner was never created in production builds.

Now loadState() runs first, state.networkId has the correct persisted value, and currentNetwork() correctly evaluates whether we are on a testnet.

docker build . passes (all 49 tests pass, lint clean, build clean). Rebased onto main.

**Rework complete — fixed banner load ordering bug.** Moved `await loadState()` and `applyTheme()` to **before** the banner creation block in `init()`. Previously, `currentNetwork()` was called before `loadState()`, so `state.networkId` was still the default `"mainnet"` — meaning `net.isTestnet` was always `false` and the testnet banner was never created in production builds. Now `loadState()` runs first, `state.networkId` has the correct persisted value, and `currentNetwork()` correctly evaluates whether we are on a testnet. `docker build .` passes (all 49 tests pass, lint clean, build clean). Rebased onto `main`.
clawbot added
needs-review
 and removed
needs-rework
 labels 2026-03-01 20:36:39 +01:00
clawbot removed their assignment 2026-03-01 20:36:43 +01:00
clawbot commented 2026-03-01 20:38:43 +01:00
Author
Collaborator
Copy Link

Independent Review — PASS

Reviewer: clawbot (independent review agent)

Summary

All review points verified. docker build . passes (format, lint, tests, build all green).

Verified

  1. Load ordering fixed: await loadState() and applyTheme() now run at the top of init(), before currentNetwork() is called. This fixes the bug from the previous review where state.networkId was still the default "mainnet" when the banner decision was made — the testnet-only banner now works correctly in production builds.

  2. Banner text matches sneak's instructions exactly:

    • Debug + testnet: "DEBUG / INSECURE [TESTNET]"
    • Testnet only: "[TESTNET]"
    • Debug only: "DEBUG / INSECURE"

  3. No extra elements: Simple textContent assignment only — no <span> children, no flexbox, no absolute positioning.

  4. showView() in helpers.js updated with same logic; appends (viewName) suffix only in debug mode.

  5. Scope: Only 2 files changed (src/popup/index.js, src/popup/views/helpers.js). No test weakening, no config changes, no unrelated modifications.

  6. Branch is up to date with main — no rebase needed.

Marking as merge-ready.

## Independent Review — PASS **Reviewer**: clawbot (independent review agent) ### Summary All review points verified. `docker build .` passes (format, lint, tests, build all green). ### Verified 1. **Load ordering fixed**: `await loadState()` and `applyTheme()` now run at the top of `init()`, before `currentNetwork()` is called. This fixes the bug from the [previous review](https://git.eeqj.de/sneak/AutistMask/pulls/143#issuecomment-9058) where `state.networkId` was still the default `"mainnet"` when the banner decision was made — the testnet-only banner now works correctly in production builds. 2. **Banner text matches [sneak's instructions](https://git.eeqj.de/sneak/AutistMask/pulls/143#issuecomment-8966) exactly**: - Debug + testnet: `"DEBUG / INSECURE [TESTNET]"` - Testnet only: `"[TESTNET]"` - Debug only: `"DEBUG / INSECURE"` 3. **No extra elements**: Simple `textContent` assignment only — no `<span>` children, no flexbox, no absolute positioning. 4. **`showView()` in `helpers.js`** updated with same logic; appends `(viewName)` suffix only in debug mode. 5. **Scope**: Only 2 files changed (`src/popup/index.js`, `src/popup/views/helpers.js`). No test weakening, no config changes, no unrelated modifications. 6. **Branch is up to date with `main`** — no rebase needed. Marking as **merge-ready**.
clawbot added
merge-ready
 and removed
needs-review
bot
 labels 2026-03-01 20:38:53 +01:00
sneak was assigned by clawbot 2026-03-01 20:39:03 +01:00
sneak added 1 commit 2026-03-01 21:55:21 +01:00
Merge branch 'main' into fix/issue-140-debug-banner
All checks were successful
check / check (push) Successful in 25s
Details
27c96ef624
sneak merged commit 39db06c83d into main 2026-03-01 21:55:36 +01:00
sneak deleted branch fix/issue-140-debug-banner 2026-03-01 21:55:37 +01:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
bot

Agent's turn to work

  
merge-ready

   
needs-checks

   
needs-rebase

   
needs-review

   
needs-rework
No Label
bot
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
sneak
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: sneak/AutistMask#143
No description provided.
Delete Branch "fix/issue-140-debug-banner"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?