Uh oh!
There was an error while loading. Please reload this page.
State restoration and continuity across devices - #5663
Open
shai-almog wants to merge 16 commits into
Open
Conversation
A Codename One app that the operating system kills comes back to its first screen. Lifecycle.stop() kept the current Form in a plain field, so a suspend and resume looked right and a reclaimed process lost everything -- which on Android is the ordinary outcome of a few minutes in another app. com.codename1.continuity saves what the user was doing and brings it back, and on Apple platforms offers that same work to the other devices the person is signed in to. The substrate was already here and unused: com.codename1.router keeps a stack of deep-link paths, which is exactly a serializable, portable "where the user is", and Navigation.restoreStack rebuilds it without animating through every screen on the way. Two packages, because they cost different things. com.codename1.continuity buys a native define and one NSUserActivityTypes entry and no entitlement; com.codename1.continuity.sync buys the iCloud key-value store, whose entitlement has to be granted on the App ID. Handing that to an app that only wanted to pass work to the tablet in the user's other hand would fail its codesigning for a capability it never asked for -- the same split, for the same reason, as usesSmartHome and usesHomeAccessoryData. Three decisions worth recording: - Saving is continuous, not at shutdown. Every navigation schedules a checkpoint that is written once per event loop pass. Android's generated activity blocks the platform main thread until the app's stop() returns, so an app that saved there would pay for it on every suspend. - Nothing happens until the application opts in. start() is unchanged for every existing app, and restore() is never called for anyone -- where restoration belongs in a launch is a decision only the app can make. - Codename One runs no relay server. Continuation between Apple devices is the platform's; everything else goes through a StateRelay against the app's own endpoint, because deciding which saved states belong to the same person is the app's account system's question. The iOS delegate matches continuity BEFORE intents, and the order is load-bearing: the intents block ends in a general branch that hands any remaining activity to Java and returns Java's answer, and Intents.dispatchUserActivity correctly declines a type it never declared. An app using both would have had its own continuation asked about by the wrong framework, told no, and dropped. NSUserActivityTypes stays a single key for the same reason a second one is worse than none: iOS reads a duplicated key unpredictably, so the two contributors meet in userActivityTypesKey. Android needs nothing injected -- no permission, no manifest entry, no dependency -- and the bridge exists there for one job: flushing the checkpoint from onSaveInstanceState, the last callback guaranteed before a background process is reclaimed. Both cross-device capabilities report themselves unsupported rather than being emulated, because an app told "yes" by a bridge that then dropped the state is worse off than one told "no", which can fall back to a relay and reach an iPhone as easily as another Android. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:def44c429d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
CollaboratorAuthor
Compared 12 screenshots: 12 matched. |
Contributor
Developer Guide build artifacts are available for download from this workflow run:
Developer Guide quality checks: |
A review asked for a LibraryClassPrefixScan fold-in over buildinRes, on the premise that a cn1lib using only continuity would be invisible to the class scan. The premise is false: CN1BuildMojo merges every compile-classpath element into one jar-with-dependencies and submits that as dist.jar, blacklisting only codenameone-core and java-runtime, so a cn1lib's classes reach the server merged into the application's own and are walked by this scan. The fix would also have been actively harmful rather than merely redundant. Navigation calls Continuity.routeStackChanged, so the framework's own classes name this package, and LibraryClassPrefixScan filters only classes INSIDE the scanned prefix -- a fold-in would have reported continuity usage for every application ever built and demanded an iCloud entitlement that fails codesigning wherever the App ID never enabled it. Comment rather than a reply, because the next reader is in the file and not in the thread. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:e9431117ba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Contributor
Cloudflare Preview
|
CollaboratorAuthor
Compared 151 screenshots: 151 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
CollaboratorAuthor
Compared 166 screenshots: 166 matched. Benchmark ResultsDetailed Performance Metrics
|
CollaboratorAuthor
Compared 166 screenshots: 166 matched. Benchmark ResultsDetailed Performance Metrics
|
CollaboratorAuthor
Compared 166 screenshots: 166 matched. |
CollaboratorAuthor
Compared 166 screenshots: 166 matched. |
CollaboratorAuthor
Compared 166 screenshots: 166 matched. Benchmark ResultsDetailed Performance Metrics
|
CollaboratorAuthor
Compared 181 screenshots: 181 matched. |
…cycle Ten findings, each with a test that fails without its fix. The sharpest was measured rather than taken on trust, and is worse than it read: JSONParser returns every JSON number as a Double and `true` as the STRING "true", so a payload crossing the relay or an NSUserActivity came back with different types than it left with -- Integer 3 as 3.0, Boolean.TRUE as "true", and Long 9007199254740993 as 9.007199254740992E15, a different number. An application casting back what it stored got a ClassCastException on Android and the desktop and, on iOS, silent corruption: ParparVM does not throw for a failed cast, it hands the wrong object to the next instruction. Payload scalars now cross as tagged strings and are rebuilt on arrival; strings are tagged too, so an application's own "i:5" is still a string. Null is no longer admitted. A property list cannot carry one: the iOS sanitizer dropped a null-valued entry and dropped a null LIST ELEMENT, shifting every index after it, so what arrived on the other device was a different shape from what was sent. Refused where the key is known instead. Relay publishes are serialized and coalesced behind one worker. A thread per checkpoint raced, and because a publish REPLACES the stored document the slower older request could land last -- leaving the user's other device fetching work they had already moved past, with nothing logged. The test drives six checkpoints through a slow relay; before the fix it observed [2, 5, 4, 3, 7, 6]. maxAge now applies to states arriving from elsewhere and to a parked one. A relay hands back whatever it still holds, so an expired checkout could auto-restore -- the exact harm the knob exists to prevent. Dropping an expired state deliberately does not consume its sequence, or a fresher state from the same device would look like one already seen. The Android suspend checkpoint runs on the event thread. onSaveInstanceState is Android's main thread, and StateProvider.saveState is documented as EDT code captured beside an EDT-owned route stack. Gated on a pending checkpoint first, so the ordinary suspend -- where write-through already ran -- still costs no thread hop, and bounded so a wedged event thread cannot turn a missed checkpoint into an ANR. The legacy delegate now compiles for continuity. application:continueUserActivity: was guarded on universal links or intents alone, so an ios.uiscene=false build using only continuity had the branch compiled and nothing to call it. Verified by preprocessing the generated project with intents off: the entry point appears only with this change, and the file compiles clean in both configurations. An AppState is now a snapshot. setPayload copied only the outer map, so nested lists stayed shared with the application and could be edited while the relay serialized them on a background thread. capture() persists the sequence it allocates. Only checkpoint() did, so an application using the documented capture() for its own transport restarted lower and had its states silently ignored by a receiver still holding the old mark. The signing preflight keys on the sync declaration rather than on a general "uses continuity" one, which warned projects about an entitlement their build was never going to request. That hint then had no reader at all, so it is gone rather than left inert. Two findings are answered in code rather than followed: - Payload-only restore keeps returning false. Returning true would leave an application whose provider only populates fields -- the shape the guide recommends -- on no screen at all. StateProvider's javadoc was the piece that disagreed with the guide, the sample and the test; it is corrected. - The cn1lib scan, answered in the previous commit. Also fixed under the project's PMD gate, which forbids volatile: the cross-thread fields are behind a lock, as CodenameOneImplementation already does it. Converting them surfaced two that were genuinely wrong -- `dirty` is read from Android's main thread, and `waitingForWindow` is cleared by the waiter thread. Removing an identity comparison in the same pass fixed a third bug: a newer state arriving while the waiter slept was being discarded in favour of the one it was started for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CollaboratorAuthor
Compared 160 screenshots: 160 matched. Benchmark Results
Detailed Performance Metrics
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:85e4b48ba4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Two fixes, one of them a break I put in CI. StateCodec used Long.valueOf(String), Integer.valueOf(String) and Double.valueOf(String). Core is compiled a second time against Ports/CLDC11 and translated against vm/JavaAPI, and neither carries the String-taking overloads -- only valueOf(primitive) -- so the Maven build accepted them against the full JDK and the Ant leg refused them: "incompatible types: String cannot be converted to long". Now X.valueOf(X.parseX(s)), which both replacements define. The local gate that catches this is `ant -f CodenameOne/build.xml compile`, and it reproduces the CI message at the same line in twelve seconds. Not running it is what let this reach CI; the fast Maven compile check cannot see it by construction. The merge now targets the LIVE activity array. A project that kept an old declaration commented out above its real one had the ids inserted into the comment: the caller correctly saw a live key, and the merge then found the dead one first. The plist that shipped had no continuity type in the array iOS reads, so Handoff was never advertised and nothing said so. This is the case the previous commit recorded as deliberately unhandled, on the grounds that it was rare and the fix meant threading comment-aware offsets through the merge. Reviewed again and that was the wrong call: firstLiveIndex plus insideComment is twenty lines, and it also covers the expander, which had the same hole. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:2c5a39af91
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…ut() lying Three findings, and two of them are holes in fixes from the previous round. The single-publisher handoff still allowed a stale final value. The finally cleared `publishing`, released the lock, and only then re-queued what it found -- so a checkpoint landing in that gap started a second publisher, and the re-queue overwrote its newer state with the older one. The relay's last value was stale and, as before, nothing said so. Observing no work and standing down now happen under one hold of the lock and there is no re-queue path at all. clear() left the relay queue alone, which is worse than an ordering bug. It is the documented logout path, and RestStateRelay reads getToken() when the request RUNS rather than when it was queued -- so a state belonging to the account that just signed out would have gone out under the next account's bearer token. Queued work is dropped and a publishEra counter makes a publisher that is midway through a request stand down instead of taking the next one. What that cannot do is recall a request already on the wire; the javadoc says so rather than implying otherwise. SyncedStore.put() answered true whenever a store merely existed. The SPI method was void and the iOS bridge swallowed every failure, so the fallback this project's own guide recommends -- write locally when the synced write fails -- could never run, and a value the store refused was reported as saved. The SPI now returns an outcome, and the native reports synchronize plus a read-back. Deliberately NOT claimed: that this detects a full store. synchronize answers about the store, not about one value, and whether iCloud goes on to propagate a value is not knowable from inside the call. The read-back establishes that the value is retrievable now, and the documentation says only that. One finding is answered in code rather than followed. A review read CodenameOne_GLSceneDelegate's willConnectToSession as never forwarding connectionOptions.userActivities for the main iOS scene, and asked the app delegate's launch-options block to cover the default cold launch. It does forward, at the end of the same method -- and the ordering problem that path really does have is solved on the Java side, where IOSContinuityCallbacks holds an activity arriving before setCallback and delivers it when Continuity.enable() installs one. The launch-options block is the legacy lifecycle's path only, and now says so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:ecf68fc364
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Two findings, both the same shape as ones already fixed here, which is the part
worth recording.
Inbound dispatch had the two-step race the relay publisher had. Recording the
high-water mark and reaching the event queue are separate steps, and two
channels deliver on threads of their own -- so an older state could pass the
dedup, pause, and be queued BEHIND the newer one that overtook it. The event
thread then restored the newer state and overwrote it with the stale one.
Rechecked inside the runnable now: a delivery that lost the race to the queue
drops itself rather than undoing the winner.
That is the third instance of "checked at step one, acted at step two" in this
feature -- the relay publisher, its restart path, and now this. Each was found
by someone enumerating an interleaving rather than by a test, so the class is
not claimed to be exhausted.
fromMap manufactured a state out of nothing. A relay answering "{}", or an
activity arriving with no usable userInfo, produced a default AppState that the
continuation callback then CLAIMED and delivered -- running the application's
listeners, and for an app that prompts before moving the user, putting a
"continue what you were doing?" dialog in front of them over no state at all.
The javadoc already promised null for a document carrying nothing recognizable;
now it does that. One known field is enough, so a state that is only routes is
still a state.
Both are pinned by tests that fail without their fix: reverting the recheck runs
the superseded delivery (2 dispatches instead of 1), and reverting the field
check returns AppState{routes=0, payload=0, device=, seq=0} where null is
expected.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:646d895af1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
… failed state Three findings, all in code written in the last two rounds rather than in the original feature. A relay poll had no logout guard. Publishing got an era check last round; polling did not, and polling is the direction that puts the PREVIOUS account's work on a user's screen. A fetch in flight when the user signs out was delivered into the next account's session -- and clear() empties lastSeen, so nothing downstream would recognise it as stale. The era is captured before the request and rechecked before delivery. Renamed to accountEra, because it governs both directions and no longer only publishing. The era check on the publish side stranded the new account's state. After a logout during an in-flight request, a checkpoint from the newly signed-in session sits in the pending slot; the check then cleared `publishing` and returned without starting a replacement, so that state waited for some later checkpoint to restart the worker. The check was unnecessary as well as harmful: clear() empties the queue, so anything present afterwards belongs to the session that is signed in now and has to be sent. A failed publish dropped the state, against StateRelay.publish's own documentation. It is kept now -- but keeping it was not enough, and this is the part worth recording. publishToRelay had exactly one caller, checkpoint(), and a checkpoint OVERWRITES the pending slot with its own newer state before starting anything, so a retained state could never be sent by any path. The first version of this fix stored it where nothing could reach it, and the first version of the test asserted that the SECOND checkpoint arrived -- which happens whether or not the first was retained, so it passed with the fix reverted. startPublisher is now separate from publishToRelay and pollRelay calls it too, which is the reconnect an application already makes on resume and the one Android makes for it. The test polls with no second checkpoint and asserts the ORIGINAL sequence arrives; reverting the retention fails it with "the retained state never reached the relay ==> expected: <1> but was: <0>". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:3f2b72c7e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
… full type Five findings. Two of them are the third and fourth variant of one bug, which is the part worth recording. "Find this key's array" had been an ad-hoc forward search, patched twice: once for a self-closing <array/>, once to skip commented-out declarations. Codex then found a comment BETWEEN the key and its array, and an unbounded search reaching past a NON-array value into a later key's array -- inserting the continuity type and every intent id into an unrelated property. The probe prints it plainly: without the fix, logWorkout and com.example.app.continuity land inside SomethingElse's array. Patching a fifth variant would have been the wrong move. immediateValueIndex walks whitespace and live comments after </key> and then STOPS, which is what a plist parser does, so the expander and the merge now share one definition of the key's value instead of three approximations of it. clear() mutated lastSeen outside the monitor deliver() and isStillNewest() hold it under. That is a data race on a HashMap, not just a stale read, and the benign-looking version let a pre-logout high-water mark survive long enough for a queued delivery to dispatch the previous account's state after logout. reset() already did this correctly, so it was an inconsistency here rather than an oversight about concurrency. The simulated synced store indexed keys in a newline-separated string, so a key containing a newline came back as two phantom keys that nothing could remove. The platform store imposes no such rule, so the simulation must not either: entries are escaped rather than the API narrowed. The iOS cold-launch callback claimed any activity whose type merely ENDS in ".continuity", which an App Intent id may also do -- skipping the intents branch for an activity this framework then discarded on delivery. It now declines only on a POSITIVE mismatch: asking for the expected type can fail before the stub has published package_name, and reading "cannot tell" as "not ours" would decline continuity's own cold launch, which is worse than the bug. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:7e121fdcef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
…structurally Three review findings, each with the reasoning kept at the line it applies to because a PR thread is not read again once the branch moves on. A delivery is two steps -- reach the event queue, then dispatch -- and testing `enabled` at dispatch cannot separate them. An application that disabled and re-enabled before the queue drained had the pre-disable arrival pass the check and restore anyway. A delivery era, bumped by disable() and clear() under the monitor the newest-sequence check already holds, makes the two questions one predicate: stillDeliverable(state, era). That subsumed isStillNewest, whose removal is not tidying -- a private method left uncalled fails the SpotBugs zero-findings gate. The account era is now re-read as late as the lock allows before the relay is invoked. With clear() draining the pending slot, the eras partition cleanly: queued is dropped, dequeued-but-unsent stands down here, in flight is unrecallable and says so, and a failed state re-checks before it is re-queued. immediateValueIndex resolves a plist key through plistKeyEnd rather than searching for the literal "</key". A key may carry a comment, a comment may contain that text, and the raw search then ended the key inside it, decided the value was not an array, and dropped every activity type silently. pollRelay keeps starting the publisher without waiting for it. Serializing the fetch behind the publication was asked for and would be worse than the race it closes: a relay holds one document per user, so the GET would read back our own POST every time and the other device's state would be overwritten before it was ever seen. The early return is this device's own echo, which deliver() already drops. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:1185b5b3f2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…eck age Three review findings, all three real. Reasoning kept at the line it applies to rather than in the PR thread, which nobody reads once the branch moves on. Applying an inbound stack is not the user navigating. restoreStack() reaches routeStackChanged(), which checkpoints, which republished the state we had just received under THIS device's id and a fresh sequence -- so the device that sent it could no longer recognize its own work. It arrived there as a foreign state, was restored, was published back, and the two devices bounced the same stack between them, re-navigating the user on every poll. An applyingRestore flag suppresses the checkpoint for exactly the duration of the restore, and the applied state is persisted locally so a cold start still lands where the user actually is. A second test pins the other half of the rule: navigation after a restore must still checkpoint, or a device that received a state once would go silent for the rest of the session. pollRelay() now runs one fetch at a time. A relay holds one document per user, so two overlapping GETs can return different states, and nothing downstream re-orders them: lastSeen is keyed by the ORIGINATING device, so a response that left first and returned second passes deduplication on its own key and paints the older screen over the newer one. Requests that arrive during a fetch are coalesced rather than dropped, because an application polling on reconnect is asking a real question. The body moved into pollOnce() first -- it had three early returns, and each would have leaked the in-flight flag and silently stopped polling for the life of the process. dispatch() rechecks the age. Arrival was not the only way in: a continuation that cold-launches the app is parked and waits up to WINDOW_WAIT_MILLIS for the first form, and the waiter then dispatched directly, past both the inbound check and the one in getRestorableState(). A state fresh on arrival that expired during that wait was restored anyway, which is precisely what maxAge exists to refuse. Probes, since a passing test proves nothing on its own: disabling the suppression fails both restore tests, and disabling the single-flight guard turns six polls into seven concurrent fetches (expected 1, was 7) -- the race was unbounded fan-out rather than the two-request inversion reported. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:5eaf1db562
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…re, scope to root Three review findings, all real. The first is a bug this branch introduced one commit ago, which is worth saying plainly: the pre-send era check added there cleared `publishing` and returned. publishToRelay() queues a state, sees publishing == true, and leaves it for the live worker on the understanding that a live worker always drains the slot -- so a checkpoint made on the NEW account right after a clear() was stranded until something else happened to start a publisher. The check now continues the loop instead, and the loop's first block re-dequeues under one lock and stands down properly when there is nothing left. The previous message claimed the eras "partition cleanly"; it had verified that no state is SENT under the wrong account and not that every queued state is still sent at all. SyncedStore.addChangeListener now resolves the platform store. enable() installs the Java callback, but on iOS the external-change observer is created the first time cn1ContinuityStore() runs, and nothing on the listener path reached it: an application that only registered a listener and waited to read values inside the callback was never told about a change made on another device, until some unrelated read or write happened to bring the store up. NSUserActivityTypes is now resolved at the plist root only. The fragment ios.plistInject supplies is a sequence of the root dictionary's own members, but a member's value may itself be a <dict> -- and a key inside one belongs to that dictionary, not to the plist. iOS reads this key at the root and nowhere else, so a nested one was merged into a dictionary nobody reads it from AND suppressed the root key that advertises Handoff: silently inert, with an unrelated property quietly rewritten. Detection, the merge and expandEmptyUserActivityArray all use the same root-scoped lookup, because the third resolves the same key and scoping only the two the review named would still have rewritten a nested <array/>. Probes: removing the store resolution fails the listener test, and neutralizing the dict-depth check fails three plist tests in each builder. The publisher fix has no test -- it needs clear() to land between two adjacent lock holds with no code between them, so there is no hook -- and is verified by reading only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:34598c5aa2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
CollaboratorAuthor
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
CollaboratorAuthor
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
The five findings first, then the reason there kept being findings. A synced-store listener no longer enables continuity. com.codename1.continuity.sync is a package of its own so its cost is earned separately, and enable() is not a cost it asks for: it makes every route change checkpoint, and a checkpoint advertises the app's navigation to the devices around it over Handoff. An application that wanted a key/value store its user's devices share was opted into broadcasting its route stack. installSyncedStoreCallback() installs only the inbound seam; the store notification does not consult `enabled`, so the listener still works. setRelay() invalidates the work of the relay it replaces. A state retained after a failed send was published to the REPLACEMENT endpoint -- an application's data sent somewhere it was never handed to -- and a poll started against a relay the app had removed could still deliver its answer. An oversized payload string is refused up front, naming the key. Util.writeObject writes every String with writeUTF, which throws past 65535 bytes; persist() logged that and carried on, so the checkpoint looked successful and was simply absent after the process died -- state restoration failing silently at the one moment it exists for. The limit counts bytes, not characters. plistDictDepth and the key lookup go through skipMarkupBefore, the scanner this file already had. A hand-rolled comment skip read "<![CDATA[a > <dict>]]>" as real nesting, so a root NSUserActivityTypes looked nested and a SECOND one was appended -- and plistWithoutComments read a "<!--" inside CDATA as an unterminated comment and truncated the fragment, hiding a live key the same way. That helper's own javadoc warns against exactly the heuristic I reproduced next to it. Now the cause. This class had 20 mutable statics, THREE locks and 33 synchronized regions -- and `enabled`, `relay` and `maxAge`, which the application writes and the relay worker and the platform's continuation thread read, had neither a lock nor volatile. The comment above them claimed "a lock rather than volatile fields". There was no lock. That is not a missing guard on one field, it is the absence of a memory model: "can these two steps interleave" had a different answer depending on which of the three locks each step happened to take, so defects arrived one interleaving at a time and each fix added another flag or another era counter, widening the surface that produced the next one. One lock now covers every mutable static, and nothing calls out -- to a provider, a listener, a relay or the EDT -- while holding it; that second half is what keeps one lock from being a deadlock, and a scripted audit reports zero violations. The single deliberate exception is getDeviceId() reading Preferences under the lock, because generating the id outside it lets two threads each mint a UUID and the device id then changes across a restart, which makes every state this device sent look foreign. It is commented as the exception it is. The unification also closed two races nobody had reported: enable() was a check-then-set and could install two callbacks, and flushScheduled was a check-then-set and could schedule two flushes for one cycle. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:f730882180
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…nstances Every finding on this branch has had one of two shapes, so this sweeps both surfaces by enumeration instead of patching what was reported. Cross-thread state with no memory model. IOSContinuityCallbacks had four statics and zero synchronization: the platform hands a continuation over on its own thread -- on a cold launch, before the EDT has run init() -- while setCallback runs on the EDT, so an arrival parked by one was not guaranteed visible to the other, and the take-and-clear was not atomic with the store. Either drops the continuation, which is the single thing that class exists to prevent. The same treatment as Continuity: one lock, and a re-read of the callback before parking so an arrival that races an enable() is delivered instead of stranded for a setCallback that has already been and gone. LocalContinuityBridge had the same shape and it was live -- the simulator's Simulate menu runs on the AWT thread and read fields the EDT writes, so it could report "nothing to deliver" for a state just checkpointed. Bespoke markup scanning. immediateValueIndex stepped over whitespace and comments but not processing instructions, so "<key>NSUserActivityTypes</key><?note?><array/>" resolved to the "<?" and both the expansion and the merge decided the value was not an array and dropped every activity type. The array dedup was a raw contains, so "<!-- <string>x</string> -->" counted as declared and the type was never added -- Handoff silently not advertised. Both now go through skipMarkupBefore, which this tree already had for exactly this and whose javadoc warns against the heuristic the hand-rolled versions kept reproducing. Validating the reported instance instead of the class. Payload strings were capped; routes, title and deviceId were not, and all four reach Util.writeUTF. externalize() then threw on a long route, persist() logged it and carried on, and the checkpoint was published to the other device while silently absent from local storage. All four surfaces are checked now. The preflight no longer returns early when the project names its own key-value container. A profile granting NO store at all fails codesigning whichever container is named, so that was the one answer it could give for certain and it was suppressing it; the unanswerable question is WHICH container, and that is still where the check stops. The existing test asserted the old behaviour and is replaced by two: granting profile stays quiet, non-granting profile warns and names the container. Probes: each fix fails its own test when reverted. The two lock fixes have no executed test -- neither the iOS callback path nor the AWT/EDT interleaving is reachable from this harness -- and are verified by reading. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:33e4b0d530
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
… Catalyst slice Four findings. Two are windows this branch opened itself, and saying so is the point of the comments at each line. A poll coalesced behind setRelay() now uses the replacement relay. The worker kept the relay it was started with and refreshed only the era, so the second attempt fetched from the endpoint that had just been REPLACED and stamped the answer with the new era -- which made the era check, whose entire job is to stop that, wave it through and restore the old endpoint's data. Relay and era are read as a pair, on every attempt, which is the only way the two can agree. enable() publishes `enabled` last, under the same hold as the state it depends on. The lock refactor moved the Preferences load out from under the lock -- right, and nothing slow belongs there -- but left the flag being set first, which turned a benign ordering into a publishable one: a second caller saw enabled, returned, and checkpointed against an uninitialized sequence of 0, writing sequence 1; this thread then restored the loaded value and the next checkpoint reused 1. A receiver holding that high-water mark discards the second state as one it has already acted on, so a real update never arrives and nothing says so. Nested map keys are length-checked. They reach the same writeUTF as top-level ones, and this is the third round of the same class: payload strings, then routes and title and deviceId, now nested keys. The validator names an oversized key without reproducing it, since the key is the thing being reported. The iCloud key-value store entitlement reaches the Catalyst slice. A Catalyst archive is signed with the plist MacNativeBuilder writes, and that plist is built from the macNative.entitlements.* namespace alone -- so the entitlement the iOS side generates reached the iOS slice and silently missed the Mac one, leaving NSUbiquitousKeyValueStore with no container in the Mac slice of the very build that switched the shared code on. It reads the value the iOS side already resolved rather than taking a hint of its own: there is one correct container per app, and a second place to configure it is a second place for the slices to disagree. Probes: the relay rebinding, the nested key and the Catalyst entitlement each fail their test when reverted. The enable() ordering has no test -- reproducing it needs a thread to observe the flag inside a specific window, and a probabilistic test is not something this repo tolerates -- so it is verified by reading. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:dca8d2d773
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
CollaboratorAuthor
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Three findings fixed, one pushed back on, and a test of my own that proved nothing. Delivery high-water marks survive a restart. `lastSeen` is process-local, so a relaunch emptied it and the next poll -- automatic on an Android resume -- accepted the same (device, sequence) again and restored a foreign state a second time, prompting the user on every launch against a documented guarantee that an acted-on state acts once. The stored checkpoint already carries the id and sequence of whatever was last acted on, including a state restored from another device, so enable() seeds from it rather than persisting a second structure. A reconnect that lands during a failed publish is honoured. startPublisher() saw publishing == true and left the work to the live worker, which is right for ordering -- but a worker whose attempt then FAILED requeued and stood down, forgetting the request, so a single reconnect after a failed send left the retained state unsent until some later checkpoint happened. The request is recorded and consumed once: one extra attempt per external call, not the spin the stand-down exists to avoid. capture() and checkpoint() run on the EDT. Both read the EDT-owned navigation stack and call StateProvider.saveState(), which is documented to run there, and both are public and cheap enough that calling them from a network callback is ordinary -- so they read the stack while the EDT mutated it. Marshalled with a BOUNDED wait, because the desktop EDT blocks on the AWT thread while painting and an unbounded wait could deadlock the two: a missed checkpoint beats a hung app. Not fixed, deliberately: a review asked this builder to resolve the iCloud container from the raw ios.entitlementsInject fragment as well. True in the BuildDaemon twin, which is where it is fixed -- and false here, because this builder never reads that hint (zero readers; the daemon has three), so locally the fragment reaches no plist and both slices already use the same value. The reasoning is at the line, next to the VPN entitlement that documents the same asymmetry. And a correction: the restart test as first written passed with the fix reverted. deliver() dispatches through callSerially and the test body IS the EDT, so it asserted zero whether the state had been dropped or was still queued. It drains the queue now, and fails with the seeding removed. The probe is the only reason that was caught rather than shipped as evidence. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:740dfdc0e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
CollaboratorAuthor
Apple TV (tvOS / Metal)Compared 144 screenshots: 142 matched, 2 missing actuals.
|
…ckpoint, persist marks Four findings, every one of them a defect in code this branch added in the last two rounds. The Catalyst slice was signed for a DIFFERENT iCloud container than iOS -- by the fix that was supposed to stop exactly that. $(CFBundleIdentifier) is target-relative, and DERIVE_MACCATALYST_PRODUCT_BUNDLE_IDENTIFIER makes the Catalyst bundle id "<package>.maccatalyst" (the same derivation the provisioning-profile block already relies on), so copying the expression verbatim produced TEAM.<package>.maccatalyst against the iOS slice's TEAM.<package>. The iOS identifier is materialized now. $(TeamIdentifierPrefix) is left alone: same team in both targets. A checkpoint that overlaps clear() is abandoned. Building the snapshot is not quick -- it calls the application's saveState() -- and the state is not in pendingPublish yet, so clear() can neither drop it nor stamp it with the old era. Persisting would recreate the storage clear() just deleted, the continuation would re-advertise the signed-out account's work, and the relay publish would go out under the next account's credentials. The era is taken before the snapshot and rechecked before the side effects. Delivery high-water marks are durable and per-device. The previous shape reconstructed at most one id from the stored checkpoint and recovered NONE once a local navigation had overwritten it, so a duplicate from any other device still arrived and still restored. They are written on every acted-on state -- from deliver() and from restore(), because an application may apply a state itself -- bounded to 64 devices, and cleared by clear() so a signed-out account's marks cannot suppress the next account's deliveries. refreshBridge() reinstalls a sync-only callback. An app that only registers a SyncedStore listener keeps continuity off deliberately, so testing `enabled` meant the simulator's capability menu -- which swaps the bridge and calls this -- left the replacement with no callback, and every later "Change the Synced Store" did nothing. Three things my own testing caught, worth recording. The materialization NPE'd when getPackageName() was null, which surfaced as a test ERROR rather than a failure. Replacing the checkpoint-derived seed with the durable map dropped the restore() path and took four tests red -- the third time on this branch that a wider mechanism silently lost a case the narrow one covered. And the durable marks leaked between tests, because they outlive reset() by design, which silently stopped three unrelated deliveries until the setup cleared them. rememberSeen() guards only the preference write. Iterating a generic map compiles to checkcasts, and the catch(Throwable) I first wrapped the whole loop in is a handler ParparVM never runs -- CHECKCAST expands to nothing there, so a failed cast crashes natively instead. check-cast-semantics flagged 7 of them; the baseline is untouched at 191. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:6e208b5384
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return; | ||
| } | ||
| } | ||
| deliver(fetched); |
There was a problem hiding this comment.
If clear() runs after this method releases STATE_LOCK but before deliver(fetched) executes, the previous account's response is admitted using the new deliveryEra and the now-empty lastSeen, so it can be restored into the next account. Fresh evidence in this revision is that the new accountEra validation remains a check-then-act sequence rather than passing the captured era into the atomic delivery admission; associate the fetched state with its poll era through deliver() so logout cannot rebrand it as a current-session arrival.
Useful? React with 👍 / 👎.
Comment on lines
+88
to
+91
| type = pendingType; | ||
| json = pendingJson; | ||
| pendingType = null; | ||
| pendingJson = null; |
There was a problem hiding this comment.
On an iOS cold launch, if SyncedStore.addChangeListener() runs before Continuity.enable(), it installs Continuity.Callback and reaches this block while continuity is still disabled; the callback declines the pending Handoff activity, but these assignments have already erased it and setCallback() ignores the false result. A subsequent enable therefore cannot recover the launch activity, so initialization order alone silently loses the continuation; clear the pending slot only after the installed callback claims it.
Useful? React with 👍 / 👎.
| applyingRestore = true; | ||
| } | ||
| try { | ||
| shown = Navigation.restoreStack(routes); |
There was a problem hiding this comment.
When application code calls the public restore(AppState) from a network or worker callback, this invokes Navigation.restoreStack() directly on that thread even though the method's own contract requires the EDT; StateProvider.restoreState() is likewise documented to run on the EDT. This can construct and show forms while the UI thread is active, causing races or platform UI-thread failures, so the entire explicit restore should be synchronously marshalled in the same way as capture/checkpoint.
Useful? React with 👍 / 👎.
Comment on lines
+1595
to
+1598
| Map<String, Long> copy; | ||
| synchronized (STATE_LOCK) { | ||
| copy = new HashMap<String, Long>(lastSeen); | ||
| } |
There was a problem hiding this comment.
When two inbound channels accept states concurrently, each call can snapshot lastSeen here and then write outside the lock; an older snapshot containing only device A can consequently be written after a newer snapshot containing A and B. The in-memory marks remain correct until restart, but B's mark then disappears and its state is acted on again, so version or serialize these preference writes to prevent stale snapshots from overwriting newer ones.
Useful? React with 👍 / 👎.
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 freeto 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.
What this is
A Codename One app that the operating system kills comes back to its first screen.
Lifecycle.stop()kept the currentFormin a plain field, so a suspend and resume looked right and a reclaimed process lost everything -- which on Android is the ordinary outcome of a few minutes in another app.com.codename1.continuitysaves what the user was doing and brings it back, and on Apple platforms offers that same work to the other devices the person is signed in to.The substrate was already here and unused:
com.codename1.routerkeeps a stack of deep-link paths, which is exactly a serializable, portable "where the user is".Navigation.restoreStackrebuilds it without animating through every screen on the way.Two packages, because they cost different things
@Routescreen stackStateRelayStateRelayStateRelayStateRelaycom.codename1.continuitybuys a native define and oneNSUserActivityTypesentry, and no entitlement.com.codename1.continuity.syncbuys the iCloud key-value store, whose entitlement has to be granted on the App ID -- handing that to an app that only wanted to pass work to the tablet in the user's other hand would fail its codesigning for a capability it never asked for. Same split, same reason, asusesSmartHome/usesHomeAccessoryData.Three decisions worth recording
stop()returns, so an app that saved there would pay for it on every suspend.start()is unchanged for every existing app, andrestore()is never called for anyone -- where restoration belongs in a launch is a decision only the app can make.StateRelayagainst the app's own endpoint, because deciding which saved states belong to the same person is the app's account system's question.The load-bearing detail
The iOS delegate matches continuity before intents. The intents block ends in a general branch that hands any remaining activity to Java and returns Java's answer, and
Intents.dispatchUserActivitycorrectly declines a type it never declared -- so an app using both would have had its own continuation asked about by the wrong framework, told no, and dropped.NSUserActivityTypesstays a single key for the same reason a second one is worse than none: iOS reads a duplicated key unpredictably. The two contributors meet inuserActivityTypesKey.Android needs nothing injected -- no permission, no manifest entry, no dependency. The bridge exists there for one job: flushing the checkpoint from
onSaveInstanceState, the last callback guaranteed before a background process is reclaimed. Both cross-device capabilities report themselves unsupported rather than being emulated, because an app told "yes" by a bridge that then dropped the state is worse off than one told "no", which can fall back to a relay and reach an iPhone as easily as another Android.Verification
The load-bearing one: a real Xcode build of the generated project produced one
NSUserActivityTypesarray carrying both the sample's three App Intent ids and...hellocodenameone.continuity.BUILD SUCCEEDEDfor iOS and watchOS, and a deliberate probe inside the#ifdef CN1_USE_CONTINUITYblock failed the build, so that check is not vacuous. Dead-code elimination left both native callbacks with real bodies.Also green locally: 46 core unit tests, 10 plist-merge tests, 7 preflight tests, 5 shipped-hooks tests; SpotBugs 0 findings across
core-unittests,android,ios,codenameone-maven-pluginandbuild-hint-catalog; and the cast-semantics, control-character, copyright, build-hint, snippet and prose gates.Paired change
The builder half is mirrored in BuildDaemon; that PR has to land with this one.
🤖 Generated with Claude Code