Version 8 cleanup#174
Open
UnknownJoe796 wants to merge 27 commits into
Open
Conversation
Multi-agent review of version-8 covering core view architecture, reactivity, theming/models, navigation/l2, and platforms/build/testing. Prioritized P0-P3 roadmap with verified file:line references. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The file previously existed only in the gitignored buildSrc mirror (regenerated by copying from gradle-plugin/src), so a git clean would have destroyed it. Move it to gradle-plugin/src/main/kotlin/ (the mirror's source of truth) and wire registerAiDriverTasks(project) into KiteUiPlugin.apply, which its own doc comment already documented as the intended call site. Registration is lazy and no-ops for consumers without an :ai-driver-server project. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
DependentAction is a DependencyChangeListener (a CoroutineContext.Element), but startAction launched the action coroutine with context = extraContext, omitting `this`. The reactive await()/state() functions locate the listener via currentCoroutineContext()[DependencyChangeListener.Key], so with the listener absent from the context, no dependencies were ever registered and onDependencyChange/onDependencyNotReady never fired. Since DependentAction is the default returned by Action(...), every button that clears its error on dependency change was silently broken. Add `this` to the launched coroutine context. Add a regression test verifying a dependency change clears the error state. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s, dead-container log - working/loading extension props cached a per-element FalseWhenSuccessful wrapper into the subtree-shared ElementContext.addons under a constant key, so every sibling element read the first element's process status. The wrapper is trivial; construct it per access. - Clear labelFor/describedBy on shutdown; they hold strong references to arbitrary elements and could keep a whole removed subtree (and its native views) alive. - Replace the bare println in checkIsShutdown with a proper Log.warn including the view path. Kept the no-op-return behavior (a teardown race can legitimately reach it) but made it visible instead of a silent stderr print. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…empty nav stubs - ColorPicker.bindColor compared .green twice and never compared .alpha, so alpha-only changes within tolerance were dropped. Compare alpha as the fourth channel. - Graph drew two println(minX/maxX) debug lines on every single canvas draw. Removed. - Removed navLayout and navSideBar from AppNavV2: empty no-op public functions with zero in-repo usages (navBottomBar, which has a real body, is kept). Source-compat note: any external caller was already getting a no-op. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
onNewIntent decoded the entire key=value pair before splitting on '=', so a value containing an encoded '=' (%3D) would corrupt the key/value split. Split on the raw delimiter first, then decode each half — matching the JS urlLike() behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PageNavigatorBehavior.Separate (the default) kept a HashMap<UrlLikePath, List<Page>> that grew unbounded for the session's lifetime, strongly retaining every Page of every stack ever visited. Replace with a bounded (50-entry) insertion-ordered map with oldest-first eviction and recency refresh on store; beyond the cap an old URL simply reparses into a fresh single-page stack. Note: the non-default Link mode still has a separate dead-code issue (its history-sync reactiveScope sets suppressNav=true immediately before an if(!suppressNav) guard, making the body unreachable) and never cleans its localStorage main-stack-* entries. Collapsing the three history strategies to one is a design decision left for maintainer review; not changed here. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- MIGRATION.md: status changed from 'IN PROGRESS/BROKEN/target version-7' to landed on version-8; removed the nonexistent CanAddDynamicTheme, added the real CanAddListElementModifier stage; replaced all '-' operator modifier examples with the actual dot-chaining syntax; withUnrestrictedModifiers -> withUnsafeModifiers. - CLAUDE.md: dot-chaining in Modifiers/Theming examples; jvmRun -> ssrServerRun. - README.md: Kotlin badge 2.2.0 -> 2.3.20. - docs/TESTING_GUIDE.md: RContext -> ElementContext. - docs/rview-basics.md: prominent notice that RView/RContext/ViewWriter are historical; points at the current Element/NativeElement/ElementWriter model. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…names
Content-neutral renames (Kotlin filename != class name, so no code changes):
- RContext.{ios,android,commonHtml}.kt -> ElementContext.<platform>.kt (they contain the
ElementContext actuals)
- RView.commonHtml.jvm.kt -> FutureElement.commonHtml.jvmSsr.kt (contains FutureElement +
FutureElementStyle/Attributes)
- Deleted RViewTest.kt, which was 100% commented-out dead code referencing the removed
RContext.test() API.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
generateRoutes.kt string-scans Kotlin sources; several defects fixed: - @QueryParameter scan was unbounded to EOF, so the first @Routable in a file collected a later class's query params. Bound the scan to the class body via a new brace-matching afterBraces() helper in parsingHelpers.kt. - Property-name extraction used indexOf("va"), matching the "va" inside `private` etc. Replaced with a word-boundary regex \bva[lr]\b. - Silent break on malformed input (missing path string, unclosed literal, no class/object) dropped the rest of a file's routes; now throws with file path + offset (fail-fast). - Route precedence was filesystem-walk-order dependent (parse takes firstOrNull). Parsers are now sorted specificity-first (fewer variables win), alphabetical tiebreak, and structurally-duplicate route templates throw with both class names. Adds GenerateRoutesKtTest (5 golden tests) covering per-class query attribution, keyword matching, ordering, duplicate detection, and malformed-input errors. Reviewed and verified :gradle-plugin:test green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The theming core previously had zero tests. Lock in the pure model-level invariants so future refactors (deduplicating the Theme setter, converting lambda derivations to Semantics) can't silently break them: - ThemeAndBack.plus OR-combines drawBackground/padding flags (ThemeRules.md contract) - SemanticOverrides precedence: instance override > type override > default - Theme.copy revert propagation (non-cascading sets earliest revert; cascading propagates) - Theme equals/hashCode are id-based (invisible contract), copy chains ids, customize does not - theme[semantic] memoization returns the identical cached instance All 24 pass against current behavior. Invariants needing an element-level harness (cascading no-child-refresh optimization, full ThemePipeline ordering) are noted as out of scope in the file header. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
All commented-out of settings.gradle and unbuilt; the swing modules still referenced the removed RView class and could not compile. Recoverable from git history if ever needed. - library-swing (80 files), library-lottie-swing, library-camera-swing, example-app-swing (only referenced each other; removed the commented includes from settings.gradle.kts) - example-app/src/wasmJsMain (6 files) — orphaned; example-app declares no wasmJs target - test-utilities/src/jvmDesktopMain — unwired (jvmDesktop target commented out per KMP single-JVM-target limitation) - library/src/commonJvmMain Example.kt — stray 2-line file in a wrong package; the commonJvmMain source set itself stays (it carries jvmSsr deps) Verified :library and :example-app jvmSsr compile clean after removal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Introduces elementTree { } — a live SSR-backed element tree for tests to build a subtree
via the normal DSL and inspect it (themeAndBack/drawBackground/theme.id, shown, children,
findByName, live-instance count, shutdown). Fills the gap where theming/view invariants
could only be tested at the pure-model level.
Adds 6 tests locking in element-level theme cascade: card draws background, plain child
inside a card does not, important child switches theme and does, nested cards each draw,
redundant refreshTheming is a no-op, and shutdown returns the live-instance count to
baseline (no leak). Prerequisite for safely landing the themeAndBack setter dedup.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ement duality)
Element extended both KiteUiCoroutineScopeHelpers (-> CoroutineScope) and StatusListener
(-> CoroutineContext.Element), so every element was simultaneously a CoroutineScope AND a
CoroutineContext.Element - and NativeElement placed the element into its own coroutineContext
as that StatusListener. That dual identity is the root of the element.job ambiguity in the
Element.kt TODO (child jobs mis-parented).
Fix: Element is now only a CoroutineScope, not a StatusListener. NativeElement supplies a
SEPARATE StatusListener object into its coroutineContext (delegating to the element's own,
now-plain, watch{Background,Foreground}Process methods). Verified nothing consumes
StatusListener via the Element type - all consumers read it from the coroutine context.
This surfaced a latent bug: TelemetryContext.viewPath() called element.outermostElement.viewPath()
which had been silently binding to CoroutineContext.viewPath() (because Element was a
CoroutineContext) and always returning an empty string. It now correctly resolves to
Element.viewPath(); added the import.
Verified: :library:jvmSsrTest green; android/js/ios compile clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Back (Android system back; browser back to follow) should dismiss the top open dialog before popping the page. Adds an app-global stack of dismiss lambdas on ElementContext (shared via the root context, since lazyContextAddon defaults store there): - pushDismissableDialog(dismiss)/dismissTopDialog() in ViewContextExtensions.kt - dialog() registers a dismissable dialog and funnels back/tap-outside/programmatic close through one idempotent dismiss that also unregisters from the stack - Android handleOnBackPressed dismisses the top dialog before mainNavigator.goBack() iOS needs no change (modal UIViewController + no hardware back). The browser popstate guard lands with the Navigator.js history rewrite. Source-compatible: dialog() signature unchanged; alert()/confirmDanger() get back-to-dismiss for free; dismissable=false dialogs stay put. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replaces the sprawl (forEach, forEachUpdating, forEachById, forEachByIdWithoutAnimation, forEachAnimated, childrenLazyLoading, Recycler2.children) with one renderList family: - keyed overload (ID diffing, animate flag, live Reactive<T>) and unkeyed overload (positional slot reuse with placeholders + a poolCap that bounds the hidden-view pool - the old forEachUpdating grew it unboundedly) - Recycler2.renderList / renderListMultipleTypes harmonize the virtualized entry point (virtualization stays separate by necessity) All old functions become @deprecated(ReplaceWith(...)) delegating to renderList, so behavior is identical (they were all @InternalKiteUi). lazyColumn/lazyRow now call renderList internally. Trivial in-repo callers migrated. Adds RenderListTest (8 tests, incl. poolCap eviction). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Recycler2PullToRefreshTest and Recycler2TestPage both claimed @Routable("recycler2-test"), so one page was silently unreachable (the pre-hardening codegen picked one by filesystem order). The new fail-fast duplicate detection caught it. Give the pull-to-refresh page its own route so both are reachable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Behavioral + structural coverage for the interactive elements, using the driver-backed
uiTest {} harness and the elementTree {} structural harness. Each file covers the standard
template: renders, binds content/value, enabled/disabled reflects action presence, action
fires (click/toggle/setValue), theme applied where expected, and no leak after shutdown.
Elements: Button, TextInput, Checkbox, Switch, ToggleButton, Select, Slider, TextArea,
NumberInput (67 tests). Adds assertEnabled/assertDisabled to UiTestScope and findByDebugName
to the element harness. Two test bugs found while verifying and fixed here (the 500ms Action
frequency cap swallows a rapid second click; a slider must set max before min to avoid a
transient inverted range) - both worth remembering when writing element tests.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…sses dialogs Collapses the three PageNavigatorBehavior strategies to just Separate (each navigate() = one browser history entry, closest to native back semantics). Link and Deprecated are kept as @deprecated no-op aliases and PageNavigatorUseExperimentalBehavior is an inert deprecated setter, so nothing external breaks. Removes the dead Link-mode code (the suppressNav guard that made its history sync unreachable) and cleans up the localStorage keys those modes left. - reset() (stack shrinks to 1) now clears browser forward/back via history.go(-(length-1)) + replaceState, so back can't return into the old stack. Detected as size==1 && prev>1. - Browser back dismisses the top open dismissable dialog first (popstate intercept + re-pushState so back keeps working), matching the Android back behavior. - Keeps the bounded lastStackForPath cache. Compiles clean (:library:compileKotlinJs). NB: browser back/forward/reset behavior cannot be verified headlessly and needs a manual browser pass (see checklist in the PR/notes). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
iHoonter
requested changes
Jul 16, 2026
iHoonter
left a comment
iHoonter
left a comment
Collaborator
There was a problem hiding this comment.
There's a bunch of comments with things to change, also I implemented navClosable on overlay to make it more generic, but it still needs to be implemented on ios.
| if (quoteStart == -1) break | ||
| // Fail fast: every @Routable must carry a quoted path string. | ||
| if (quoteStart == -1) throw IllegalStateException( | ||
| "@Routable at offset ${match.index} in ${file.path} is missing its path string (expected a quoted argument)" |
Collaborator
There was a problem hiding this comment.
Unnecessary, would be a compiler error
| */ | ||
| @InternalKiteUi | ||
| fun <T> ContainerElement.forEach( | ||
| fun <T, ID> ContainerElement.renderList( |
Collaborator
There was a problem hiding this comment.
what if someone needs to have their own shownWhen logic in a list? With the internal renderers being private there's currently no way to do this without using the @UnsafeModifier escape hatch.
| @@ -22,37 +22,37 @@ fun <T, ID> ElementWriter.colOf( | |||
| preHidingModifiers: ViewWriter.(ID) -> ElementWriter.CanAddListElementModifier = { this }, | |||
Collaborator
There was a problem hiding this comment.
preHidingModifiers are discarded, should be used. Also every col/rowOf variant should have a beforeModifier now.
| @InternalKiteUi | ||
| fun <T> ContainerElement.renderList( | ||
| items: Reactive<List<T>>, | ||
| animate: Boolean = false, |
Collaborator
There was a problem hiding this comment.
unused parameter, I'm guessing it should be used, seems like a good thing to have control over.
| * No Signal wrapper needed — the whole list is rebuilt on every change. | ||
| */ | ||
| @InternalKiteUi | ||
| private fun <T> ContainerElement.renderListSimple( |
Collaborator
There was a problem hiding this comment.
I don't think this should be private, or internal. This is used in a lot of places.
| * animate=false, no wrapper is inserted and the render lambda receives | ||
| * [CanAddListElementModifier] directly. | ||
| */ | ||
| @InternalKiteUi |
Collaborator
There was a problem hiding this comment.
I think these list components deserve their own OptIn annotation, because this isn't meant purely for internal use. Until there is a variant of colOf for every possible container, these still have a use case.
| */ | ||
| @InternalKiteUi | ||
| @Deprecated( | ||
| "Use renderList instead", |
Collaborator
There was a problem hiding this comment.
replace with renderListSimple. renderList is not a 1-to-1 replacement, and this ReplaceWith will break things.
| * @deprecated Use [lazyColumn] or [lazyRow] instead, which handle rendering + lazy loading together. | ||
| */ | ||
| @OptIn(InternalKiteUi::class) | ||
| @Deprecated( |
Collaborator
There was a problem hiding this comment.
Personally I think this is not Deprecated, it is internal, it should be marked as @InternalKiteUI and not as deprecated, or even better it should use the new opt in annotation for the renderList variants.
Also, if we're changing the name of these functions this should be renderLazyList or something similar.'
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
2 participants
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.
No description provided.