Fix 6 bugs in the Amicode settings dialog's Developer Tools / Permissions UI - #318
Merged
Merged
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The remote rebuild pulls different tracked branches for the two repos
(local/amicode for opencode, main for amicode), so no single label is
fully precise for both sides. This reverses an earlier pass at this same
label ('Pull & Rebuild', chosen because it read as accurate for both
repos) back to 'Rebuild from Main' per explicit preference — it's the
branch users actually think about when they look at this button, even
though it's not the literal branch the opencode side pulls.
TDD: extended the existing rebuild-label-rename regression test (the one
that already scans the app package for the stale 'Rebuild from Latest'
string and asserts en.ts's key value) by updating its NEW_LABEL constant
to 'Rebuild from Main' — same test, same guarantee, new target string.
i18n parity suite and typecheck both still pass.
rebuild() set the amicode:devtools-rebuilt flag at the same time as amicode:devtools-rebuilding, so reopening the settings dialog while a rebuild was still running showed 'Rebuilt!' instead of 'Rebuilding...'. Worse, simply removing that premature set (without a corresponding fix) would have regressed the success case entirely: the dev-tools-rebuild-status 'done' handler only ever cleared 'rebuilding' — it never set 'rebuilt' — so after a successful rebuild, onMount would see no flags at all and silently fall back to 'idle'. Extracted the flag lifecycle (start / done / failed) into a pure rebuildFlagMutation() function in developer-tools-rebuild-flags.ts, so the protocol is unit-testable without a SolidJS reactive root or settings context. rebuild() now only sets rebuilding+reopen at start; the 'done' handler now sets 'rebuilt' (and clears 'rebuilding'); the timeout and 'failed' paths share the same clear-only mutation. TDD: wrote the pure-function tests + a localStorage integration test reproducing both the original bug (start alone must never look like 'rebuilt') and the naive-fix regression (done must actually set the flag) before the module existed — confirmed RED (module not found), implemented, confirmed GREEN (7/7), then wired the controller and reran the full settings-v2 suite (9/9).
sendUpdate() blanked `status` to undefined the instant a validation round-trip started, so any visible error/success indicator vanished and then snapped back once the reply arrived — a visible flash on every path field blur. The message handler also set multiple signals (status, pending) outside a batch, risking an intermediate render between them. Modeled the request lifecycle as a pure reducer (reduceDevToolsRequest) in developer-tools-request-state.ts: 'request-sent' now keeps the stale status and only flips pending; 'status-received' replaces status and clears pending. sendUpdate() and the message handler now go through it, and the handler's signal writes are wrapped in solid-js's batch(). Since keeping stale status without any visual cue would trade flicker for silently-stale data, added a 'Validating…' indicator (bound to the existing but previously-unused `pending` signal) and dim the stale error text while a new round-trip is in flight. Also added the missing .settings-v2-field-info CSS rule the 'Building extension…' text already referenced but never had defined. TDD: wrote reduceDevToolsRequest's tests first (the key regression case: a second request-sent must keep the FIRST round-trip's status visible, not blank it) — confirmed RED (module not found), implemented, confirmed GREEN (4/4), wired the controller, reran the full settings-v2 suite (13/13), typechecked clean.
PermissionScopeSetting (the 'Auto-accept permissions' switch) lived in the General tab, but it is a permissions control — it belongs with the other permission settings, not general app preferences. Three coordinated changes make the move real: - general.tsx: removed the component definition, its controller instantiation, and its render call, plus the now-unused createPermissionScopeController/PermissionScopeController imports. - permissions.tsx: added the component (moved verbatim), instantiates createPermissionScopeController with the tab's own sessionID prop, and renders it at the top of the tab body, above the trust tiers. - dialog-settings-v2.tsx: threads sessionID into SettingsPermissionsV2, which never received it before (the controller needs it to resolve the active session's auto-accept state). TDD: no @solidjs/testing-library render harness exists in this codebase, so — following the same source-scanning pattern already used elsewhere (the 'outer relay script forwards preview-file to the iframe' structural test) — wrote a test asserting the move via source content: general.tsx no longer references PermissionScopeSetting/createPermissionScopeController, permissions.tsx does, and the dialog's permissions tab content passes sessionID. Confirmed RED (4/4 failing — nothing moved yet), performed the move, confirmed GREEN (4/4). Typechecked clean; full app suite still passes (1103/1103).
.devtools-section-header lays the 'Developer Tools' title and the rebuild status indicator side by side with align-items: center. But .settings-v2-section-title carries a padding-bottom sized for the normal case (a title sitting above a settings list), which shifts its visual center upward relative to the status text beside it — the title reads as sitting slightly higher than 'Rebuilding...' / 'Rebuilt!'. Added the same padding-bottom: 0 override the .settings-v2-providers and .settings-v2-models section titles already apply for the identical reason. TDD: wrote a test reading amicode.css directly (following the existing pattern in wave-geometry.test.ts) asserting the override rule exists — confirmed RED (selector not found), applied the CSS fix, confirmed GREEN. Full ui package suite still passes (491/491).
The paired amicode PR removes the auto-build that used to fire on every amicode-path commit — that build was the only thing that ever set DevToolsStatus.building/buildError, so with it gone these fields, the 'Building extension…' UI text, and its i18n key across all 18 locales are permanently unreachable. Removed: - building?/buildError? from the DevToolsStatus interface and from the status-received construction in the message handler - the building() computed and its <Show> block in developer-tools.tsx - the now-dead s.buildError check in amicodeError() (falls straight through to s.amicodeError, which is what the extension actually sends) - the orphaned settings.general.row.amicodePath.building i18n key from every locale (nothing in code referenced it once the UI text was gone) Removal-driven refactor, not new behavior — verified via the full suite before (17/17 settings-v2, 1103/1103 app) and after (same counts, 0 regressions), plus a clean typecheck. i18n parity suite still passes.
jeonghun-jj-lee
force-pushed
the
fix/settings-dialog-bugs
branch
from
September 9, 2026 22:42
c2101e9 to
6e28200
Compare
The earlier padding reset removed one source of misalignment, but the header still used align-items:center. Centering the 12px status line box against the 15px title line box left the status baseline visibly lower than the heading. Aligning the flex row on its baseline is the correct typographic primitive: the text lines now share a baseline while the status dot remains centered inside its own inline status row. TDD: changed the existing CSS regression test to require baseline alignment, confirmed RED against align-items:center, changed the CSS, then confirmed GREEN. Full UI suite: 491/491 pass.
jeonghun-jj-lee
added a commit
to harmoniqs/amicode
that referenced
this pull request
Sep 10, 2026
Squash merge of #939 after all required CI checks passed. Companion fork PR harmoniqs/opencode#318 merged first into local/amicode.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Five user-reported bugs plus two found during investigation, all in the settings dialog's Developer Tools section and the Permissions tab.
dev-tools-updatehandler validated repo paths without expanding a leading~, so the default settings paths (~/harmoniqs/opencode,~/harmoniqs/amicode) always failed even when correct. Fixed in the pairedamicoderepo PR (this fork doesn't own that handler); this PR is the app-side half of the same effort.local/amicodefor opencode,mainfor amicode), not a single "latest". Renamed across all 18 locales + a stale code comment. (This went through an intermediate label, "Pull & Rebuild", chosen because it read as accurate for both repos — reverted to "Rebuild from Main" per explicit preference, since that's the branch users actually think about when they look at this button.)rebuild()set the success flag at the same time as the in-progress flag, so reopening the dialog mid-build showed "Rebuilt!" instead of "Rebuilding...". Fixing this naively (just removing the premature set) would have regressed the success case entirely, since thedonehandler never set the flag either — extracted the flag lifecycle into a pure, tested function to get both halves right.sendUpdate()blanked the status signal on every path edit, so any visible error/success indicator vanished and snapped back on each round-trip. Modeled the request lifecycle as a pure reducer; added a "Validating…" affordance so keeping stale status visible doesn't just trade flicker for silently-stale data.sessionIDthrough since the Permissions tab never received it before.building/buildErrorcode (companion to amicode#941) — the paired amicode PR removes the auto-build that used to fire on every amicode-path commit; that build was the only thing that ever set these fields. Removed the now-permanently-unreachableDevToolsStatusfields, the "Building extension…" UI text, and its orphaned i18n key across all 18 locales.Approach
Each bug is its own TDD slice: a failing test capturing the exact regression first, confirmed RED, then the minimal fix, confirmed GREEN. Two bugs (the flag lifecycle, the flicker) are modeled as pure functions extracted from the SolidJS controller — this codebase has no
@solidjs/testing-libraryrender harness, so following the existing pattern (general-controller-behavior.ts/general-controllers.test.ts), the signal-adjacent logic is pulled into plain functions that are directly unit-testable, with the controller as a thin reactive shell around them. The label rename and the structural tab-move used source-scanning regression tests, following the same pattern already established for the "outer relay script forwards preview-file" structural test. The dead-code removal is a removal-driven refactor verified via the full suite before/after rather than a fresh RED/GREEN cycle (there's no new behavior to fail against).Testing
bun test --conditions=solid --preload ./happydom.ts ./srcinpackages/app: 1103 pass, 0 failbun testinpackages/ui: 491 pass, 0 failbun turbo typecheck: 30/30 packages passNew test files
packages/app/src/i18n/rebuild-label-rename.test.tspackages/app/src/components/settings-v2/developer-tools-rebuild-flags.ts(+ test)packages/app/src/components/settings-v2/developer-tools-request-state.ts(+ test)packages/app/src/components/settings-v2/permission-scope-relocation.test.tspackages/ui/src/amicode/devtools-header-alignment.test.tsCompanion PR
Paired with harmoniqs/amicode#939 for the tilde-expansion fix and the amicode-path auto-build removal in the extension-side
dev-tools-updatehandler (that repo ownschat_bridge.ts).