jgamboaandClaude
07825c1bd1
Evaluation branch: a full Aero skin, including frameless windows with our own title bar, switchable against the existing modern look. The app already had a four-axis runtime skinning mechanism and zero component stylesheets, so the skin is one new setting plus one stylesheet section rather than a parallel UI. The key move is cascade order: 7.css is imported into a `skin` layer ABOVE Tailwind's `utilities`, so its bare element rules outrank the ~1900 inline utility classes already in the templates. Template changes are limited to additive, skin-inert hooks (ARIA roles, `has-scrollbar`, `w7-toolbar`, `w7-btn`). - styles.css: explicit layer order, `7.css/dist/7.scoped.css` via `layer(skin)`, a token bridge re-pointing the existing semantic tokens at Win7 values (no new tokens, so `@theme inline` is untouched), a hand-authored dark Win7 palette, and neutralizers that `revert-layer` 7.css's dialog-button metrics back to the utility layer. Neutralizers must live inside `skin`, not `skinfix` — `revert-layer` only reaches `utilities` from one layer up. - Frameless chrome: `chromeOptions(skin)` is now the single place either skin's frame is decided. `titleBarStyle: "hidden"` on every platform, which is also the only way to drop the macOS traffic lights. New `title-bar` component mounted by all three window roots. - Window controls over RPC (minimize/toggle-maximize/close/is-maximized), served per window kind so the X reuses each one's existing close path — editors still get their dirty prompt. electrobun has no maximize event, so the state is derived from `resize`; double-click-to-maximize is implemented here too. - Font: Noto Sans vendored (OFL-1.1) under ui/src/fonts, since Segoe UI is absent off Windows and not redistributable. Verified: backend `tsc --noEmit` clean, `bun run build:ui` clean (486 kB initial, under the 500 kB budget), app launches with the skin active and no console errors. The rendered result has not been eyeballed — screen capture and accessibility are both denied to the shell on this machine. Co-Authored-By: Claude <noreply@anthropic.com>
152 lines
9.2 KiB
Markdown
152 lines
9.2 KiB
Markdown
# Gotchas — landmines this codebase has already stepped on
|
||
|
||
Every one of these caused a real bug or debugging spiral. Read before
|
||
touching the adjacent code.
|
||
|
||
## Angular (v22, zoneless, signals, OnPush)
|
||
|
||
- **Never consume push streams through signal+effect.** An effect that reads
|
||
a signal and (directly or transitively) writes signals re-triggers itself —
|
||
this froze the entire app on the first transfer. Push messages
|
||
(transferUpdate etc.) go through plain callback registries in RpcService.
|
||
Signal coalescing would also silently drop intermediate progress snapshots.
|
||
- **No signal writes during template render** (NG0600). Anything that loads
|
||
panels from code paths reachable during render defers with
|
||
`queueMicrotask` (see `PanelsService.newWorkspace`).
|
||
- **`[value]` on `<select>` is applied before `@for` options render** and
|
||
silently selects nothing. Use per-option `[selected]` bindings (connection
|
||
manager stack picker).
|
||
- Template bindings with special characters: `[class.bg-primary/15]` works;
|
||
`[class.shadow-[...]]` (brackets) does NOT parse — use `[style.box-shadow]`.
|
||
Static class attributes may use arbitrary values freely.
|
||
- Functions called from templates ARE reactive: signals read inside them
|
||
register on the view's consumer (call-stack-based tracking). That's why
|
||
`iconClass(entry)` reacting to `settings.iconColors()` works.
|
||
- Dedicated tools > shell: IDE diagnostics between paired template/class
|
||
edits are often stale mid-sequence — trust `bun run build:ui` (full AOT
|
||
type-check), not the first red squiggle.
|
||
- Tailwind v4 canonical class names: `wrap-break-word` not `break-words`,
|
||
`accent-primary` not `accent-[var(--primary)]`, `z-60` not `z-[60]`.
|
||
Every color used in a template must resolve through `@theme inline`.
|
||
- **The win7 skin layer outranks Tailwind utilities.** 7.css is imported
|
||
`layer(skin)`, ordered above `utilities`, so any utility you add to a
|
||
`button`/`input`/`select`/`textarea`/`table` is silently dead under that
|
||
skin. If it matters, neutralize in the `@layer skin` block at the bottom of
|
||
styles.css — and note `revert-layer` only reaches `utilities` from **inside
|
||
`skin`**; used from `skinfix` it falls back onto 7.css and is a no-op. This
|
||
bit once already.
|
||
- `@import … layer(name)` works through `@tailwindcss/postcss` (it wraps the
|
||
imported AST), but the **deep path is mandatory**: 7.css ships no `exports`
|
||
map and Tailwind's resolver reads only `style`/`mainFields`, so a bare
|
||
`@import "7.css"` resolves to nothing. Use `7.css/dist/7.scoped.css`.
|
||
- Stylesheet `url()` must resolve at BUILD time: assets referenced from
|
||
`styles.css` live under `ui/src/` (esbuild emits them hashed into `media/`
|
||
and rewrites the url), NOT `ui/public/` — a public path fails the build with
|
||
"Could not resolve".
|
||
|
||
## Electrobun
|
||
|
||
- **Window `close` is not cancellable** (response type `{}`; only the
|
||
app-level `before-quit` supports `event.response = {allow:false}`). All
|
||
unsaved-state guards must either intercept app-controlled close paths
|
||
(⌘W, buttons) or survive the close (editor draft mirroring).
|
||
- **One rpc instance per window** — BrowserView binding steals the rpc's
|
||
transport; a shared instance silently sends to the wrong webview.
|
||
- **RPC before socket open is silently lost.** Everything gates on the
|
||
socket-open promise (RpcService `ready`).
|
||
- The emitter has no `off()`. Window event closures must not capture records
|
||
you want garbage collected.
|
||
- `electrobun build` defaults to the **dev** channel; release builds need
|
||
`--env=stable`. The stable app name differs from the dev one
|
||
(`miniSCP` vs `miniSCP-dev`), including the app-data dir implications when
|
||
testing.
|
||
- Tray: never call `setImage` after construction on macOS — the template
|
||
treatment is lost and the icon renders black on dark menu bars.
|
||
- `ApplicationMenu` is macOS-only in our usage; accelerators on Windows are
|
||
single-character only. ContextMenu exists on macOS+Windows, not Linux.
|
||
- **Window-open flash prevention is three-layer** — remove any one and the
|
||
white/inverted blink returns: (1) windows are created `hidden: true` and
|
||
revealed by `showWhenReady` on the webview's first `dom-ready` (800 ms
|
||
fallback), (2) the bun preload bakes the persisted `data-theme`/tone/accent
|
||
attributes before first paint (`buildPreload`), (3) `ui/src/index.html`
|
||
carries an inline `<style>` with the html background per theme — its oklch
|
||
values MUST stay in sync with the `--background` tokens in styles.css.
|
||
The win7 skin adds a **fourth value to keep in sync**: `buildPreload` also
|
||
bakes the `.win7` class, and index.html carries a matching `html.win7` arm
|
||
for all four theme states. Miss it and every win7 window flashes white.
|
||
- **`titleBarStyle` is construction-time only.** Switching skin cannot
|
||
re-frame a live window; `chromeOptions` is read once per `new
|
||
BrowserWindow`. The options row tells the user to reopen. Don't "fix" this
|
||
by recreating the main window — that would drop every session and tab.
|
||
- Dev vs prod URL: `getMainViewUrl()` resolves the ng-serve dev server only
|
||
when `DEV_SERVER_URL` is set AND the updater channel is `dev` AND the
|
||
server answers a HEAD request; secondary windows must reuse the resolved
|
||
URL, never hardcode `views://`.
|
||
- Upstream Bun panic (`FFI_Callback_threadsafe_call` → `web_worker.spin`):
|
||
rare segfault under long sessions; upstream electrobun×Bun issue, restart
|
||
recovers, nothing app-side to fix.
|
||
|
||
## Domain logic
|
||
|
||
- **sessionId ≠ filesystem identity.** Two panels on the same server are two
|
||
SSH sessions. Every same-file/nested-path guard compares `fsKey`
|
||
(`resolvedIP:port` / `"local"`); comparing sessionId caused a
|
||
truncate-to-zero data-loss bug. When fsKeys differ but paths collide, the
|
||
probe-file check unmasks aliased hosts. Never "simplify" this back.
|
||
- `createWriteStream` truncates BEFORE the source is read — that's why
|
||
self-copy is destructive, and why guards must run before any stream opens.
|
||
- The synthetic `'..'` row: type `'dir'`, must be excluded from selection,
|
||
targets(), filtering match-count, kind/rights/owner display, and drag
|
||
sources. Any new row-level feature must decide what `'..'` does.
|
||
- `Semaphore.wake()` must reserve slots synchronously; waking waiters that
|
||
increment asynchronously loses budget under concurrent release.
|
||
- Settings: the backend spread-merges (`{...DEFAULT_SETTINGS, ...data}`) so
|
||
new keys auto-default, but the UI SettingsService reads field-by-field —
|
||
a new setting silently stays at default until wired in all five spots.
|
||
- The editor draft mirror sends full content per tick (2 MB cap upstream);
|
||
don't shrink the 400 ms debounce.
|
||
- `panel.touched` gates localDir auto-navigation; `connectToken` guards
|
||
against a slow connect resolving into a panel the user already reused.
|
||
|
||
## i18n
|
||
|
||
- **Translations live in `src/shared/i18n/*.ts` (TS, not JSON)** — deliberate:
|
||
it sidesteps `resolveJsonModule` differences between the Angular esbuild and
|
||
Bun builds and gives full typing. `en.ts` is the source of truth; other
|
||
locales are partial and fall back to English then the raw key.
|
||
- **`i18n.t()` is reactive because it reads the `locale` signal** — that's what
|
||
makes live language switching work with no pipe. If you cache a translated
|
||
string in a plain field it WON'T update on switch; call `t()` in the template
|
||
(or in a signal-reading getter) instead. Same for `i18n.date()/size()`.
|
||
- Dictionary values are TS string literals: **French/other apostrophes break
|
||
single-quoted entries** (`'…c'est…'`). Use double quotes for any value
|
||
containing an apostrophe. Plurals are `{ one, other }` maps (not ICU strings);
|
||
zh/ja supply only `other`.
|
||
- **Native menus can't use the Angular service** — `menu.ts`/`context-menu.ts`
|
||
go through backend `i18n.ts`, and the app menu must be rebuilt when the locale
|
||
changes (wired in index.ts `settingsChanged`), or it keeps the old language.
|
||
- `eu`/`zh`/`ja` are AI drafts pending native review (see the i18n README).
|
||
- **Silent auto-refresh must snapshot sessionId+cwd and discard a mismatched
|
||
result.** A normal `refresh()` is masked by `busy` (input blocked), but the
|
||
silent variant (poll/watch) runs while the user can navigate — without the
|
||
guard, a poll that started in dir A resolves and stamps A's listing over the
|
||
dir B the user just opened. It also never calls `scrollCursorIntoView`
|
||
(scroll must stay put) and returns early if `busy()` or already refreshing.
|
||
|
||
## Build / tooling
|
||
|
||
- Angular CLI rejects Bun's Node-compat version — `scripts/node-version-shim.ts`
|
||
is preloaded via `bunfig.toml` at the root AND in `ui/`. Removing either
|
||
breaks `ng`.
|
||
- ssh2's optional native deps must stay `external: ["cpu-features", "*.node"]`
|
||
in `electrobun.config.ts` — bundling them breaks the build; ssh2 falls back
|
||
to pure JS at runtime.
|
||
- `bun run --cwd ui build` (flag order matters; `bun --cwd ui run build` does
|
||
not do what you want).
|
||
- `version.gen.ts` is gitignored and regenerated by postinstall + every build
|
||
script; a fresh checkout fails `tsc` until `bun install` (or any script)
|
||
has run once. Never commit it — it would bump the commit count it records.
|
||
- Icon pipeline is `scripts/gen-icons.ts` (resvg) — regenerate after touching
|
||
`assets/logo.svg` or `assets/tray/tray-glyph.svg`; the .ico and tray PNGs
|
||
are build inputs, not build outputs.
|