Skip to content

Improving Bean Shell Support in Playground#4780

Merged
shai-almog merged 116 commits intomasterfrom
feat/bsh-java-parity-matrix
Apr 22, 2026
Merged

Improving Bean Shell Support in Playground#4780
shai-almog merged 116 commits intomasterfrom
feat/bsh-java-parity-matrix

Conversation

@shai-almog
Copy link
Copy Markdown
Collaborator

No description provided.

@shai-almog shai-almog changed the title Feat/bsh java parity matrix Improving Bean Shell Support in Playground Apr 19, 2026
@shai-almog shai-almog requested a review from Copilot April 19, 2026 15:24
@github-actions
Copy link
Copy Markdown
Contributor

Cloudflare Preview

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 19, 2026

✅ Continuous Quality Report

Test & Coverage

Static Analysis

Generated automatically by the PR CI workflow.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR modernizes the Codename One Playground BeanShell fork to support more Java-like syntax and class constructs, and adds CI automation to prevent regressions in Playground language support.

Changes:

  • Replace the previous ASM/bytecode-generation approach for script-declared types with a namespace-backed ScriptedClass/ScriptedInstance runtime model and integrate it into name resolution, field access, method dispatch, and allocation.
  • Extend parser/runtime rewriting to handle additional Java syntax patterns (e.g., instanceof bindings, improved try-with-resources parsing, and additional lambda rewriting paths).
  • Add/expand smoke + syntax-matrix CI coverage and update tooling/docs to run these checks reliably in GitHub Actions.

Reviewed changes

Copilot reviewed 41 out of 41 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
scripts/cn1playground/tools/run-playground-smoke-tests.sh Switches registry checks to grep, updates Maven build/run steps, and runs both smoke + syntax matrix harnesses.
scripts/cn1playground/tools/generate-cn1-access-registry.sh Adds conditional tools.jar support to compile/run the registry generator across JDK setups.
scripts/cn1playground/common/src/test/java/com/codenameone/playground/PlaygroundSmokeHarness.java Forces process exit after successful run to avoid CI hangs due to non-daemon threads.
scripts/cn1playground/common/src/main/java/com/codenameone/playground/PlaygroundRunner.java Introduces a script “plan” that evals type declarations separately, plus new rewrite passes (inline AutoCloseable helpers, top-level lambdas).
scripts/cn1playground/common/src/main/java/bsh/cn1/CN1LambdaSupport.java Makes LambdaValue implement Runnable to improve SAM assignment compatibility.
scripts/cn1playground/common/src/main/java/bsh/ScriptedInstance.java Adds new runtime object representing instances of script-declared classes (namespace-backed fields + dispatch).
scripts/cn1playground/common/src/main/java/bsh/ScriptedClass.java Adds new runtime descriptor for script-declared classes (methods/ctors/fields, enum constants, inheritance among scripted classes).
scripts/cn1playground/common/src/main/java/bsh/Parser.java Adds instanceof pattern binding parsing and loosens try-with-resources grammar (incl. trailing semicolon).
scripts/cn1playground/common/src/main/java/bsh/Operators.java Fixes shift operator semantics so int-sized shifts don’t get incorrectly promoted to long.
scripts/cn1playground/common/src/main/java/bsh/Name.java Adds scripted-class field access and method dispatch paths (instance + static + enum built-ins).
scripts/cn1playground/common/src/main/java/bsh/BSHType.java Allows typed declarations referencing scripted classes to resolve as Object (enables assignment of ScriptedInstance).
scripts/cn1playground/common/src/main/java/bsh/BSHPrimarySuffix.java Implements scripted instance/static field access and scripted method/static method dispatch for primary suffixes.
scripts/cn1playground/common/src/main/java/bsh/BSHEnhancedForStatement.java Adds explicit null-iteratee error for enhanced for-loops.
scripts/cn1playground/common/src/main/java/bsh/BSHClassDeclaration.java Evaluates class declarations into ScriptedClass objects rather than throwing “unsupported” errors; adds basic interface/enum marking.
scripts/cn1playground/common/src/main/java/bsh/BSHBinaryExpression.java Supports instanceof binding by capturing identifier at parse time and binding on successful match.
scripts/cn1playground/common/src/main/java/bsh/BSHAllocationExpression.java Enables new against scripted classes (via namespace lookup), returning ScriptedInstance.
scripts/cn1playground/README.md Updates test instructions and documents a language feature rollout process tied to the smoke/syntax harness.
.github/workflows/cn1playground-language.yml Adds a dedicated workflow to run Playground language smoke tests (with Xvfb) on PRs and pushes.
scripts/cn1playground/common/src/main/java/bsh/org/objectweb/asm/TypeReference.java Removes vendored ASM source (part of eliminating ASM-based class generation).
scripts/cn1playground/common/src/main/java/bsh/org/objectweb/asm/TypePath.java Removes vendored ASM source (part of eliminating ASM-based class generation).
scripts/cn1playground/common/src/main/java/bsh/org/objectweb/asm/Type.java Removes vendored ASM source (part of eliminating ASM-based class generation).
scripts/cn1playground/common/src/main/java/bsh/org/objectweb/asm/Symbol.java Removes vendored ASM source (part of eliminating ASM-based class generation).
scripts/cn1playground/common/src/main/java/bsh/org/objectweb/asm/Opcodes.java Removes vendored ASM source (part of eliminating ASM-based class generation).
scripts/cn1playground/common/src/main/java/bsh/org/objectweb/asm/MethodVisitor.java Removes vendored ASM source (part of eliminating ASM-based class generation).
scripts/cn1playground/common/src/main/java/bsh/org/objectweb/asm/Label.java Removes vendored ASM source (part of eliminating ASM-based class generation).
scripts/cn1playground/common/src/main/java/bsh/org/objectweb/asm/LICENSE.txt Removes vendored ASM license file (corresponding to ASM source removal).
scripts/cn1playground/common/src/main/java/bsh/org/objectweb/asm/Handler.java Removes vendored ASM source (part of eliminating ASM-based class generation).
scripts/cn1playground/common/src/main/java/bsh/org/objectweb/asm/Handle.java Removes vendored ASM source (part of eliminating ASM-based class generation).
scripts/cn1playground/common/src/main/java/bsh/org/objectweb/asm/FieldWriter.java Removes vendored ASM source (part of eliminating ASM-based class generation).
scripts/cn1playground/common/src/main/java/bsh/org/objectweb/asm/FieldVisitor.java Removes vendored ASM source (part of eliminating ASM-based class generation).
scripts/cn1playground/common/src/main/java/bsh/org/objectweb/asm/Edge.java Removes vendored ASM source (part of eliminating ASM-based class generation).
scripts/cn1playground/common/src/main/java/bsh/org/objectweb/asm/CurrentFrame.java Removes vendored ASM source (part of eliminating ASM-based class generation).
scripts/cn1playground/common/src/main/java/bsh/org/objectweb/asm/Constants.java Removes vendored ASM source (part of eliminating ASM-based class generation).
scripts/cn1playground/common/src/main/java/bsh/org/objectweb/asm/ClassWriter.java Removes vendored ASM source (part of eliminating ASM-based class generation).
scripts/cn1playground/common/src/main/java/bsh/org/objectweb/asm/ClassVisitor.java Removes vendored ASM source (part of eliminating ASM-based class generation).
scripts/cn1playground/common/src/main/java/bsh/org/objectweb/asm/ByteVector.java Removes vendored ASM source (part of eliminating ASM-based class generation).
scripts/cn1playground/common/src/main/java/bsh/org/objectweb/asm/Attribute.java Removes vendored ASM source (part of eliminating ASM-based class generation).

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


echo "Verifying Component is present in generated registry..."
if ! rg -q 'index.put\("com\.codename1\.ui\.Component"' "$ROOT/common/src/main/java/bsh/cn1/GeneratedCN1Access.java"; then
if ! grep -q 'index.put("com.codename1.ui.Component"' "$ROOT/common/src/main/java/bsh/cn1/GeneratedCN1Access.java"; then
Copy link

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grep patterns here are interpreted as regex, so the unescaped . characters in com.codename1... will match any character. This weakens the smoke test and could allow false positives. Use fixed-string matching (e.g., grep -Fq) or escape dots to ensure the exact class names are verified.

Copilot uses AI. Check for mistakes.
Comment on lines +162 to +166
String importSection = normalized.substring(packageEnd, importEnd);
List<String> declarationScripts = new ArrayList<String>();
StringBuilder remainingBody = new StringBuilder();
int cursor = importEnd;
for (int i = 0; i < declarations.size(); i++) {
Copy link

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

importSection is built from normalized.substring(packageEnd, importEnd), which drops any leading package ...; declaration. When you later eval each top-level type declaration separately, it won't include the package statement, which can change semantics for scripts that declare a package. Consider including the full prefix up to importEnd (package + imports) when building declarationScripts.

Copilot uses AI. Check for mistakes.
Comment on lines +62 to +64
* <p>Interfaces and enums are still rejected — they require richer
* runtime support (SAM binding, singleton instances) that is layered
* on top of the basic scripted-class core.
Copy link

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This Javadoc says interfaces/enums are “still rejected”, but the implementation now builds a ScriptedClass, marks isInterface/isEnum, and (for enums) populates constants. Update the comment to reflect the current behavior (e.g., interfaces are declarable but not instantiable; enums have limited support).

Suggested change
* <p>Interfaces and enums are still rejectedthey require richer
* runtime support (SAM binding, singleton instances) that is layered
* on top of the basic scripted-class core.
* <p>Interfaces and enums also use the {@link ScriptedClass} machinery.
* Interfaces may be declared and can expose static and default methods,
* but they are not directly instantiable. Enums have limited runtime
* support: the declaration is represented as a scripted class marked as
* an enum, and its constants are populated during evaluation.

Copilot uses AI. Check for mistakes.
shai-almog and others added 6 commits April 21, 2026 06:40
Updates the BeanShell-Interpreter-Tradeoffs section to reflect:
* sealed/permits is now runtime-enforced, not just stripped
* records support compact constructors
* pattern-switch statements with type bindings are supported
* Collection.stream() routes through CN1StreamBridge with the full
  filter/map/reduce surface (no Optional, BinaryOperator-keyed reduce)
* "Did you mean" suggestions are emitted for missing static
  fields/methods so typos surface fixable hints

Non-static inner classes remain a documented gap.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Switch the three lookup tables (CLASS_INDEX, METHOD_INDEX,
FIELD_INDEX) to the initialization-on-demand holder idiom. Before,
a single <clinit> on GeneratedCN1Access eagerly built all three —
paying the METHOD_INDEX and FIELD_INDEX cost on the first class
touch even though those tables are only used by the
"did you mean" diagnostic path.

After, the FIELD_INDEX and METHOD_INDEX classes stay cold until a
suggestion is requested. findClass still forces the CLASS_INDEX
holder, but the diagnostic-only indexes are deferred.

Benchmark (cold JVM): first findClass ~120ms (same), first
getFieldNames now ~6ms (previously amortised into findClass).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mark nested non-static class declarations as inner on their
ScriptedClass (setEnclosingClass pointing at the outer). A
ScriptedInstance now also carries an optional enclosingInstance
reference so inner instances can walk out to the outer's
instance namespace.

ScriptedClass.newInstance gains an overload that accepts an
enclosing instance. The new instance's namespace is parented off
the enclosing namespace rather than off the outer's static
namespace, so outer instance fields resolve inside inner methods.

BSHAllocationExpression walks the callstack at `new Inner()` to
find a `this` whose ScriptedClass matches (or transitively
encloses) the required outer. Constructing a non-static inner
class when no enclosing `this` is in scope fails fast with a
clear diagnostic.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rors

A concrete (non-abstract) class that declares `implements Iface` for
a Java interface must now provide every method declared on that
interface. The check runs at class-declaration time and lists the
missing method names in the diagnostic. Catches the
`class Other implements ActionListener {}` gotcha where the user
meant to override actionPerformed but forgot.

Missing-method errors on scripted instances now surface a
"Did you mean: X?" hint drawn from the class's declared instance
methods, or list a few known method names when no close match is
found. Same Levenshtein-close / prefix match heuristics used by the
static-field suggestions.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The earlier enforcement pass only covered Java interfaces — a
concrete class implementing a scripted interface with an abstract
method slipped through because the interface's methods are merged
into the class's instance-method table during build, so a naive
name-presence check saw them as "provided". Real concrete classes
should still have to supply a body.

MethodTemplate now exposes isAbstract() based on whether the
declaration has a blockNode. ScriptedClass gains a
getConcreteInstanceMethodNames() view that filters out bodyless
merged interface stubs, and getAbstractInstanceMethodNames() so
the check can enumerate what must be implemented.

BSHClassDeclaration.enforceInterfaceMethodImplementations now
walks both Java and scripted interfaces: missing names fail with
"class X is not abstract and does not implement all methods from
scripted interface 'Iface'. Missing: ...". Default methods still
satisfy the requirement; explicit empty bodies (void foo() {})
count as concrete.

Two new matrix cases: unimplemented scripted-iface method is
rejected; scripted-iface default method is inherited cleanly.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
BSHAllocationExpression.constructFromEnclosingInstance now handles
scripted inner-class allocation: when the outer is a
ScriptedInstance, look up the named inner class in its static
namespace, verify it's non-static, verify the outer's type is
assignable to the inner's required enclosing class, then
newInstance(args, outer). Static nested classes and Java outer
instances are rejected with clear diagnostics.

Root findClass no longer builds a giant Map<String, Class<?>>
eagerly. Instead it derives the package name from the lookup and
dispatches to the matching per-package GeneratedAccess_* helper
(which itself uses a lazy if-chain over class literals). Nested
class lookups walk back through dotted prefixes until a known
package matches. Only the package whose class is asked for gets
its helper loaded.

Cold-start impact (median of 3 fresh JVMs):
  before: first findClass 120 ms, first ui getStaticField 18 ms
  after:  first findClass  27 ms, first ui getStaticField  8 ms

PlaygroundColdStartHarness is checked in so regressions are
visible — run it via `java -cp ... com.codenameone.playground.
PlaygroundColdStartHarness` from a fresh JVM.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@liannacasper liannacasper force-pushed the feat/bsh-java-parity-matrix branch from a84ece3 to 7b207c1 Compare April 21, 2026 09:02
…start

Reflect the features that actually ship on CN1's runtime surface:
* Non-static inner classes (including outer.new Inner()).
* Abstract-method enforcement at declaration time for both Java
  and scripted interfaces.
* Instance-method "did you mean" hints in addition to the existing
  static-field ones.
* PlaygroundColdStartHarness as a regression baseline.

Also tightens the non-goals list: JDK surface that isn't in CN1's
runtime (Optional, List.of, Map.of, Set.of, Stream.of,
IntStream.range, extended Collectors, etc.) is explicitly out of
scope — the playground mirrors CN1's actual API surface so
scripts that compile here also run on device.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@shai-almog shai-almog self-assigned this Apr 21, 2026
shai-almog and others added 20 commits April 21, 2026 16:09
Parse errors now carry three things the old format lacked:
* precise location pulled from the ParseException's currentToken
  (line + column, not just line)
* an "unexpected <token>" tag so the user sees exactly what broke
* the offending source line with a caret marker under the bad
  column — easier to eye-find than a line number in a long script

Token-aware hints replace the generic "check for mismatched
braces" line: mismatched parens, braces, commas, semicolons, and
stray keywords like `else`/`catch`/`finally` each get a tailored
hint pointing at the likely cause.

Matrix grows by ~20 cases:
* 4 parse_diagnostics cases pin the new formatter's output so
  regressions in location / hint / EOF-handling surface in CI.
* extended/ category covers CN1-supported surface that wasn't
  previously exercised: String methods, StringBuilder chaining,
  Integer/Math APIs, Arrays.asList iteration, HashMap entrySet
  walk, Integer.toHexString, button-listener chaining, nested
  try/catch/finally with rethrow, enhanced-for over int[], ternary
  chains, bitwise ops, indexed charAt iteration, ArrayList sort
  with Comparator, three-level class hierarchy with super chain,
  enum with abstract per-constant method.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Enhanced-for over int[], long[], double[], float[], short[], byte[],
char[], boolean[] and String[] now works via dedicated
primitive-array iterators in CollectionManager. The previous
implementation only iterated Object[], so a bare
  for (int x : new int[]{1,2,3}) ...
used to throw "Only Object[] arrays are supported".

BshArray.getIndex / setIndex / arrayLength pick up the same
dispatch: indexed reads return the boxed wrapper, indexed writes
unwrap Numbers/Characters/Booleans back into the primitive slot.
Callers that used to cast `(Object[]) obj` in BSHPrimarySuffix
(`.length` field access and subscript length-derivation) now go
through BshArray.arrayLength so primitive arrays report their
actual length.

CN1's reduced reflection surface forbids Float.TYPE / Short.TYPE /
Character.TYPE / Boolean.TYPE, so primitive-array reads now return
raw boxed wrappers (Integer.valueOf(x), Character.valueOf(c), ...)
instead of going through Primitive.wrap with a primitive-type
class literal.

Matrix grows by 26 cases:
* 7 primitive-array enhanced-for variants covering all eight
  primitive types plus String[].
* extended/ category now covers CheckBox/RadioButton/Slider/
  ComboBox, TextField/TextArea, layout variations (BorderLayout
  regions, GridLayout, FlowLayout), style API, Calendar field
  setters, Math.sqrt+floor, HashMap put/remove, generic holder
  class, class field holding an int[], iterator.remove, nested
  enhanced-for, recursive method, short-circuit boolean eval.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reports of the preview keeping pixel artifacts from the previous
run's rendering even after the new component is added. The fix:
call previewRoot.repaint() after revalidate() to force a full
redraw of the preview region.

revalidate() normally triggers a repaint, but on CN1 7.0.234 the
combination of an embedded Form-as-component with a subsequent
Container swap occasionally leaves the outgoing component's pixels
on screen until the user triggers another paint (typing in the
editor, scrolling). The explicit repaint() closes that gap.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two related changes that address the refresh regression reported
by users:

* Side-menu sample + history buttons now close the side menu FIRST
  and then call setScript. Previously the order was reversed:
  setScript ran, then closeSideMenu started its slide-out animation
  while the 1ms UITimer scheduled by runScript was still pending.
  The animation preempted the scheduled work, so replacePreview
  only visibly landed when the user took another action (typing,
  scrolling) that forced another paint.

* runScript now enqueues executeRunScript via CN.callSerially
  instead of UITimer.timer(1, ...). callSerially respects the EDT
  event queue — it runs after the current animation frame
  completes, so the preview swap happens AFTER the side-menu close
  animation settles, not during it.

Together these resolve the symptom where selecting a sample from
the side menu left the previous sample's rendering on screen until
the user typed into the editor.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The repeating UITimer(250ms, true, form, syncOpenSideMenuTheme)
was stomping on CN1's side-menu close animation: mid-slide, the
animation fades the panel's transparency down from 255, and every
250ms the timer would force bgTransparency back to 255. The panel
never actually faded out — it remained rendered, opaque, and
stuck on the form underneath.

The stuck panel produced two visible symptoms:
* "Two menus": opening the side menu showed a fresh slide-in
  panel on top of the stuck previous panel.
* "Overlay on top of the new sample": after selecting a sample
  from the side menu, the stuck panel's pixels covered the
  preview area. Typing into the editor eventually triggered
  enough EDT work that the stuck panel got cleared by CN1's
  normal paint cycle, revealing the new sample underneath.

Fix: only force the theme styling when Toolbar.isSideMenuShowing()
returns true. While the menu is closed (or mid-close), the timer
ticks become a no-op and CN1's own animation / paint cycle
finishes the slide-out cleanly.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This is aimed at the JS-port-specific rendering bug where the
previous sample's pixels remain visible as a non-interactive
background after selecting a new sample (until the user types into
the editor to force another paint cycle).

On the JavaScript port, plain Container.revalidate() on a
scrollable viewport can leave the previous component's pixels
cached in the canvas offscreen buffer when the replacement has
different bounds. forceRevalidate() is the CN1-documented "more
powerful" variant that recursively re-lays out the full hierarchy
before repainting. Chaining that with a parent revalidate walks
the invalidation one level up the tree so the SplitPane / Tabs
panel containing previewRoot also refreshes its backing surface.

The simulator build isn't affected (its Swing paint path
invalidates more aggressively), which is consistent with the
user's observation that the bug is only visible in the JavaScript
port build.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Experiment to isolate a long-standing JS-port-only rendering bug:
after a sample switch the previous sample's UI remains visible in
the background as a non-clickable layer, and opening the side menu
shows a second menu underneath the fresh slide-in. The simulator
build (which uses the TextArea fallback, not a BrowserComponent)
is unaffected, which points at the iframe peer.

The flag was originally added so that showing a Dialog (which
deinitialises the enclosing Form) wouldn't destroy the editor
iframe while the Dialog was up. The side effect is that the
HTML5Peer stays pinned in the DOM across any detach/reattach,
which is suspected to produce orphaned iframes stacked over
their supposed-to-be-current positions when CN1's JS-port peer
moves the iframe around.

Removing the flag restores CN1's default behaviour: iframes go
away on deinit and come back when the component is reattached.
Users can verify the fix by flipping to a sample that shows a
Dialog and checking whether the expected editor-reappears-after-
close behaviour still works. If that regresses and the original
symptom is gone, we have a pinpointed test case: the pin-on-
deinit flag is the root cause of the doubling, and we can
replace it with a more targeted guard.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds three commands to the toolbar's right bar, one for each of
the first three samples (Welcome, build(ctx), Lifecycle Demo).
Each button calls exactly the same setScript(sample.script, true)
path the matching side-menu entry uses — no side-menu open/close
animation, no toolbar.closeSideMenu() call. Purely a diagnostic
so we can test whether the "previous demo visible as background"
symptom reproduces without any side-menu involvement.

Verdict on this run will pinpoint whether the bug is inherent to
the script-swap path itself or specific to the side-menu
open/close cycle. To be reverted (or made conditional) once the
cause is identified.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Suspected root cause of the JS-port "previous demo visible as
background" + "two side menus on open" symptoms. Verified: a
title-bar button that calls setScript(...) without going through
the side menu doesn't reproduce the bug; side-menu buttons do.

The 250ms UITimer walked the current form's tree every tick
force-setting bgTransparency=255 / bgColor on any component with
a SideNavigation* UIID. On the JS port, running this repeatedly
while CN1 was in the middle of close-side-menu animation / DOM
teardown appears to have accumulated stale side-menu DOM elements
(the "replicated" view the user reported). Simulator (Swing) paint
path rebuilds the frame on every repaint, so the stale DOM
doesn't manifest there.

Also drops the now-orphaned syncOpenSideMenuTheme and
applySideMenuContainerTheme helpers. If side-menu dark styling
regresses visually, the right fix is to apply it via CN1's theme
props (addThemeProps in applySideMenuPalette) rather than polling.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Following the diagnostic that confirmed the side-menu path is the
trigger for the JS-port "previous demo visible as background" +
"two side menus" symptoms, pull out the remaining overrides so
CN1's own side-menu theming can run unimpeded:

* Delete applySideMenuPalette + its two callers. The aggressive
  addThemeProps of SideNavigationPanel / RightSideNavigationPanel /
  StatusBarSideMenu / SideCommand bg/border props was layered on
  top of CN1's own theme on every applyCurrentCss / applyDarkMode.
* Remove the applyWebsiteTheme walk on every side-menu component
  and the sideMenuComponents tracking list used to re-theme them
  on dark-mode toggle.
* Drop the custom PlaygroundSideCommand / PlaygroundSideCommandLine*
  / PlaygroundMenuEmpty UIID assignments on side-menu children so
  CN1's theming picks up the default SideCommand UIID chain.

Visual appearance of the side menu in dark mode will regress
(probably translucent / default CN1 side-menu look) but the
correctness regression we were chasing takes priority. Restoring
the dark-mode appearance without re-introducing the orphan-DOM
bug is a separate follow-up.

Matrix still 325/325; UI paint 312/312.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The diagnostic confirmed the bug is specific to the side-menu path,
and persists after removing all the dark-mode theming hacks. That
leaves the click-plumbing itself as the difference: the title-bar
buttons that don't reproduce the bug use CN1 Commands, while the
side-menu entries were hand-rolled Buttons added via
addComponentToSideMenu + a manual closeSideMenu in the action
listener.

Swap sample + share entries to toolbar.addCommandToSideMenu(Command)
so CN1 owns the tap -> close-animation -> actionPerformed sequence.
That's the same machinery the working title-bar buttons use. If the
ghost-preview / duplicate-side-menu symptoms clear up, it was the
manual-close order interacting badly with CN1's own close handling
on the JS port.

History entries are still MultiButton-based (they have two lines)
and use the old path — the sample path alone should be enough to
confirm whether the switch fixes the bug.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@shai-almog shai-almog merged commit a1cd3cf into master Apr 22, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants