BLD-6: add web app manifest + offline service worker to the three web… - #55
Open
patrickunterwegs wants to merge 37 commits into
Open
BLD-6: add web app manifest + offline service worker to the three web…#55patrickunterwegs wants to merge 37 commits into
patrickunterwegs wants to merge 37 commits into
Conversation
CopyWebpackPlugin resolved '../../webWorker/spectacledSqlWorker.js' against webpack's own build context, which isn't guaranteed to be 2 levels above the repo root - it only coincidentally worked for node_modules because Kotlin/JS's npm tooling hoists/symlinks it nearby. Resolve the worker file via path.resolve(__dirname, ...) instead, which is always relative to this config file's real location, and pin the copy destination for all three files explicitly via `to` so nothing depends on the plugin's default flattening behavior. Confirmed via the reported console error: the worker script itself 404'd (Worker failed to load), which is what actually caused the WebWorkerException, not anything inside the worker.
…CopyWebpackPlugin Two rounds of relative/absolute path guessing against CopyWebpackPlugin both failed - the actual root cause was that Kotlin's Gradle plugin merges every webpack.config.d/*.js fragment into one generated config file under build/js/, so __dirname inside these fragments never pointed at the real webpack.config.d directory in the first place. That's an internal build-tool detail not worth depending on. Moved spectacledSqlWorker.js into shared/src/webMain/resources instead. Kotlin's Gradle plugin copies src/<sourceSet>/resources to the web root for both the js() and wasmJs() browser targets automatically, with no custom webpack config needed - this is the standard, documented KMP mechanism other static web assets in this project already rely on, so it also directly avoids inventing more hand-written build glue than necessary. Reverted the three webpack.config.d/sqljs-config.js files back to just copying sql-wasm.wasm/sql-wasm.js (the part that was already known to work before this feature).
…ommit The previous commit only picked up the file rename because 'git add -A --' aborted on a stale pathspec - these four files (the webpack.config.d revert to just the two sql-wasm entries, and the updated comment in DatabaseDriverFactory.web.kt) belong to that same change.
…ared's shared/src/webMain/resources still 404'd - a Kotlin Multiplatform library module's JS/Wasm resources don't get bundled into a consuming app's final webpack output the same way an app module's own resources do. The proven, already-working location for static web assets in this project is each compose*App's own src/webMain/resources (that's exactly where favicon.ico, index.html and styles.css already live and are correctly served). Duplicated the worker script into all three app modules to match that existing convention instead of inventing a new one.
Clarified comment about sqljs.worker.js location and persistence.
The sequential String.replace chain in parseProperty was not a correct
inverse of escapeIcsValue: escaping "C:\Users\name" yields
"C:\\Users\\name", whose middle "\\n" the replace("\\n", "\n") pass
misreads as an escaped newline - the text comes back from the server as
"C:\Users<newline>ame". Any value containing a literal backslash
followed by 'n', ',' or ';' was affected; found immediately by the new
round-trip test suite (QUA-1) before it was even finished.
Replaced with a single left-to-right scan (unescapeIcsValue) that can
never pair a backslash with a character produced by an earlier escape,
and that also accepts RFC 5545's uppercase "\N" newline form from
other producers. Verified against a Python port of both algorithms
across plain text, all reserved chars, doubled/trailing backslashes,
and the corrupting cases.
Covers the code that is testable without a network or database, with the serialize->parse round trip through the real serializer and parser as the centerpiece - the exact path every entry takes to a CalDAV server and back: - IcalEntryRoundTripTest: journal/task/multi-entry round trips (dates, status, classification, categories, color, sequence, URL), line folding survival, TZID handling incl. VTIMEZONE emission (DST and non-DST zones), RELATED-TO with and without RELTYPE, unknown properties preserved as extraProperties, URI and inline-Base64 attachments (via a fake FileManager), VEVENT filtering, missing-UID skip, and server-style pre-folded input. - IcsEscapingAndFoldingTest: escape/unescape round trips including the backslash cases the previous unescape corrupted, RFC "\N" support, fold/unfold inverses, space and tab continuations, LF-only input. - IcsDateTimeFormatTest: all four value shapes (DATE, UTC, TZID, floating) in both directions, unknown-TZID fallback, and malformed values degrading to null instead of throwing. - IcsDateTimeTest: effectiveZone priorities, asDateOnly across a zone-boundary date change, withZone wall-time preservation. - Domain: IcalEntry progress/status mapping and share text, SyncState and CalendarSyncStatusType exhaustive state checks, Status per component, Calendar privilege helpers, CalendarSyncStatus JSON round trip with forward-compatibility, Attachment type detection. Removes the template placeholder SharedCommonTest (assertEquals(3, 1+2)), which the suite replaces. String-level expectations (escape/fold pipelines) were validated against a Python port of the exact algorithms, since no Kotlin toolchain is available in this environment - the suite still needs one local ./gradlew :shared:allTests run to confirm compilation.
…ub.com/TechbeeAT/spectacled into claude/spectacled-code-review-followup
…QUA-9)
The comma stays the separator - RFC 5545 defines it, and every other
CalDAV client (jtx Board, KOrganizer, Evolution) already writes
categories with "\," escaping, so a custom separator would break the
interop this app exists for. Instead the standard's own escape
mechanism is now applied on both sides:
- Serializer escapes each category individually, so a comma INSIDE a
value ("\,") stays distinguishable from the separators between values.
- IcsProperty now also carries the still-escaped rawValue, because the
list split has to happen BEFORE unescaping - afterwards an escaped
comma is indistinguishable from a separator. Everything except
CATEGORIES keeps using the unescaped value as before.
- New splitIcsList() splits a raw value on unescaped commas only, then
unescapes each element; the parser now also flatMaps over ALL
CATEGORIES properties instead of silently dropping all but the first
(RFC allows several per entry, and some clients emit them that way).
While wiring this up it turned out the local database had the same bug
independently: IcalEntryMapper stored categories as a plain
comma-joined TEXT column, so a fixed wire format would still have been
re-split into fragments on the next local save/load. The DB layer
(mapper both directions, getAllCategories, updateCategory) now uses the
same escapeIcsValue/splitIcsList pair, keeping one canonical escaping
implementation for both layers. Existing rows without backslashes or
commas parse identically; per owner, existing data is tester-only.
Tests: splitIcsList unit tests (incl. a category ending in a backslash
directly before a separator), a full serialize->parse round trip with
comma/semicolon/backslash categories, and multi-line CATEGORIES merge.
…ub.com/TechbeeAT/spectacled into claude/spectacled-code-review-followup
…t it (QUA-1) Closes QUA-1's remaining gap. The coordinator called the top-level webdav *Multiplatform functions directly, so its conflict-resolution logic - the most valuable untested code in the module - couldn't run without a real server. Refactor: extracted a WebDavRemoteDataSource interface wrapping the seven server operations the coordinator needs, with a production DefaultWebDavRemoteDataSource that just delegates to the existing top-level functions. SyncCoordinator's primary constructor now takes that interface; a secondary constructor keeps the exact 5-arg (client, credentials) form every existing call site already uses, so nothing else changed. Tests pass a fake and need no HttpClient at all. The (previously public) client/credentials properties are gone - grep confirmed nothing read them; all call sites construct-and-call inline. Tests (17) drive the real syncCalendarWithSyncLock entry point with a scriptable fake server and recording fake repositories, covering the push state machine (LOCAL_MODIFIED put success/conflict-server-modified/ conflict-server-deleted/not-found/failed-retry, LOCAL_DELETED delete success/conflict, USER_DECIDED_SERVER_WINS, and the SYNCED/CONFLICT "do not push" guards), the apply-server-changes machine (insert new, overwrite synced, local-modified->conflict, same-etag skip-without- fetch, server-delete->trashbin, server-delete-of-local-modified-> conflict), sync-status mapping (NOT_AUTHORIZED, sync-token-failed falling back to the tokenless REPORT), and the per-calendar lock skipping a concurrent second sync. The fake server's mutating calls default to throwing AssertionError, so any unexpected server call fails the test loudly rather than being swallowed by sync()'s catch(Exception). Adds kotlinx-coroutines-test (same version as the pinned coroutines) for runTest in commonTest.
…ub.com/TechbeeAT/spectacled into claude/spectacled-code-review-followup
Every DAV server call now goes through one of two injectable interfaces instead of a direct top-level function call, so the whole DAV surface is mockable and swappable - and there's a clean seam to lift into a standalone DAV library later. - WebDavRemoteCalendarDataSource: discovery (principals/home collections/ calendars) + calendar create/update/delete. - WebDavRemoteIcalEntryDataSource: entry sync (the two REPORTs, moved here from the calendar side where they never belonged - they enumerate a calendar's entries), single-entry fetch/put/get/delete, and attachment upload/download. Both are stateless with respect to credentials - credentials are passed per call - so the Default implementations hold only the transport (and a FileManager for inline attachments) and are registered as app-wide Koin singletons. This is what makes "inject once, call everywhere, construct nowhere" actually hold: nothing news up a data source or calls a *Multiplatform function except the two Default impls. Consumers migrated to inject the interfaces: AccountListViewModel (drops its HttpClient entirely), DetailsViewModel (keeps HttpClient only for the Claude client + SyncCoordinator), and SyncCoordinator (primary constructor now takes the IcalEntry data source + credentials; the 5-arg production constructor is unchanged, so companion/SyncTrigger call sites are untouched). Renamed discoverHomeCollections/discoverCalendars to the *Multiplatform suffix for naming consistency and to avoid a member/ top-level clash inside the Default impl. Tests: the SyncCoordinator fake now implements WebDavRemoteIcalEntryDataSource (credentials-per-call); no behavioral change.
…ositories
Both Default DAV data-source constructors take only injectable params, so
they can use the same singleOf(::Ctor) { bind<Interface>() } form the
repository singletons already use, instead of a single<Interface> { ... }
lambda. (The HttpClient binding stays a single { } lambda since it is
built from a factory method, not a constructor.)
Adds ktor-client-mock as a commonTest dependency and a WebDavParsingTest suite that drives the *Multiplatform DAV functions against scripted MockEngine responses. These cover the parsing seam the fake-interface SyncCoordinator tests deliberately skip: - sync-collection: sync-token + etag map parsing, null-etag deletion signal, relative-href resolution, REPORT/Depth/Basic-auth request shape, and 404/401/500 status mapping - multiget href report: 200-only propstat filtering - principal discovery: current-user-principal href resolution and the all-403 -> NotAuthorized path - home-collection discovery: home-set/displayname/address-set extraction - calendar discovery: component/resourcetype filtering (VJOURNAL kept, VEVENT-only dropped) and home-collection privilege extraction Also verifies credentials-per-call: a call with credentials sends Basic auth, a call with null sends none. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xt7MRuFpW1jtcHX1MPC4EN
…ones
kotlinx-datetime on the js/wasmJs targets has no time-zone database, so
TimeZone.of("Europe/Vienna") throws IllegalTimeZoneException in the browser.
That made 20 ICS timezone tests fail under :shared:allTests (they pass under
:shared:jvmTest, which uses the JDK's zones), and it also degraded the web
runtime: the ICS parser silently fell back to UTC for zoned entries, and
TimeZoneSerializer.deserialize would throw outright when loading a stored
entry with a named zone.
Add the @js-joda/timezone npm dependency to the shared web source set - the
documented remedy - which registers the tz database with the js-joda backend
kotlinx-datetime uses on both js and wasmJs.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xt7MRuFpW1jtcHX1MPC4EN
Declaring the @js-joda/timezone npm dependency was not sufficient: kotlinx-datetime
only uses it if the module is genuinely imported, and its tz-database registration
runs as an import side effect that webpack strips when nothing references it. So the
IANA database was never loaded and TimeZone.of("Europe/Vienna") kept throwing
IllegalTimeZoneException under :shared:allTests (js and wasmJs browser targets),
while :shared:jvmTest stayed green via the JDK's zones.
Add an @jsmodule external declaration for the module in the jsTest and wasmJsTest
source sets (they differ: js needs @JsNonModule, wasmJs does not) and reference it
from a real test. The reference forces webpack to emit the require, whose side
effect registers the database at module load - before any commonTest timezone test
class is constructed - so the existing ICS/date-time suite can resolve named zones
on the browser targets. Each test also asserts the database is present, guarding
against regressions.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xt7MRuFpW1jtcHX1MPC4EN
Replaces the two throwing js/wasmJs guard tests with a single expect/actual 'ensureTimeZoneDatabaseLoaded()'. The web actuals hold the @jsmodule declaration for @js-joda/timezone (js keeps @JsNonModule, wasmJs does not) and reference it so the bundler emits the import whose registration side effect runs at module load; the wasmJs actual wraps the reference in runCatching because materializing the side-effect-only module as a value throws on wasmJs (the earlier guard test failed on exactly that, even though the database had already loaded). JVM/Android/iOS actuals are no-ops. This also applies the two follow-ups: - Production web fix: the function is called from TimeZoneSerializer (init) and from parseIcalEntries, so the @js-joda/timezone import is pulled into the app bundle and named TZIDs resolve on the web target instead of silently degrading to UTC (parser) or throwing while deserializing a stored zoned entry (serializer). - Defense in depth: TimeZoneSerializer.deserialize now falls back to UTC for unknown/unresolvable zone ids via runCatching, mirroring the ICS parser, instead of throwing. A single commonTest TimeZoneDatabaseTest asserts named zones resolve on every target; on js/wasmJs its call to ensureTimeZoneDatabaseLoaded() is what pulls the import into the test bundle, before any other timezone test class is constructed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xt7MRuFpW1jtcHX1MPC4EN
…actual Replaces the expect fun ensureTimeZoneDatabaseLoaded() and its five actuals (three of them no-ops on JVM/Android/iOS) with a single web-only loader in webMain: loadTimeZoneDatabase() plus the @jsmodule declaration for @js-joda/timezone, shared by both js and wasmJs. Loading the IANA tz database is purely a web concern, so it no longer leaks into common code. Production triggers it once from SecureStorageReadyGate (the web-only startup wrapper all three apps already use), so the @js-joda/timezone import lands in the app bundle and named zones resolve at startup. The two web test triggers (jsTest/wasmJsTest) call the same loader so the import lands in the test bundles too, before any timezone test class is constructed - :shared:allTests does not run the app startup path. TimeZoneSerializer.deserialize keeps its runCatching-to-UTC fallback for unknown/unresolvable ids. The temporary calls added to the serializer init and to parseIcalEntries are removed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xt7MRuFpW1jtcHX1MPC4EN
…t bundle A transitive dependency (the ktor JS client) references Node core modules os and path in code paths the browser never runs; webpack 5 drops the old auto-polyfills and prints 'Module not found' for them when bundling spectacled-shared-test. Add a shared/webpack.config.d entry mapping os and path to false (empty module), the same resolve.fallback mechanism the app modules already use for sql.js, merged into the existing resolve config so Kotlin's aliases are preserved. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xt7MRuFpW1jtcHX1MPC4EN
Move withContext(ioDispatcher) out of the ViewModel launch sites and down into
the layer that actually does the I/O, the way the repositories already self-
dispatch, so ViewModels no longer name a dispatcher for network work:
- Both DefaultWebDavRemote*DataSource impls wrap every call in
withContext(ioDispatcher) (network, plus iCal parsing/serialization and inline
attachment file I/O on the fetch/put paths). The Compose Desktop rationale -
the Main dispatcher not resuming Ktor continuations, which was the motivating
hang - now lives here, at the layer it applies to.
- KtorRemoteGitHub{Contributor,Release}DataSource and KtorRemoteClaudeDataSource
self-dispatch their network + JSON work.
- CredentialStore (all four platform actuals) self-dispatches its KSafe reads/
writes; already suspend, so no caller changes.
ViewModel launches that were purely network/credential/repository revert to a
plain launch { }: AccountListViewModel's delete/discover/create-calendar and
DetailsViewModel's Claude request. AccountListViewModel no longer imports
ioDispatcher at all.
Deliberately left dispatched (with updated comments explaining why): AboutViewModel
(inline libraries.json parse), DetailsViewModel's sync (SyncCoordinator still reads
attachment bytes off disk directly) and the three attachment file-I/O launches, and
ListViewModel (CPU-bound recompute()). These remain because FileManager and the
UserAppPreferencesStore property accessors are synchronous by design - used in the
pure, tested ICS mapper, a composable, and theme reads - so making them suspend is
a larger, separate change tracked as the QUA-12 follow-up.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xt7MRuFpW1jtcHX1MPC4EN
…ub.com/TechbeeAT/spectacled into claude/spectacled-code-review-followup
…ktop
QUA-12 moved withContext(ioDispatcher) out of the ViewModel launches and into the
data sources + the KSafe-backed CredentialStore, so ViewModels could launch on Main.
It works on Android but breaks credential loading on iOS and Desktop: the app reads
credentials back as null, prompts to update the password, and the password entry is
then unusable.
Root cause is that the ioDispatcher-at-the-launch-site pattern was load-bearing for
reasons this refactor's premise ignored:
- CredentialStore is KSafe, a stateful store with its own cache/coroutine
machinery. It was proven working when called from inside an ioDispatcher launch;
wrapping its suspend calls in a nested withContext(ioDispatcher) (and/or invoking
them from a Main-dispatched coroutine after reverting the launches) misbehaves on
iOS/Native and Desktop while Android tolerates it - reads come back as the default
(null).
- On Compose Desktop the Main dispatcher doesn't reliably resume suspended
continuations, which is exactly why these launches were on ioDispatcher to begin
with; letting ViewModels launch on Main reintroduces that fragility.
Reverts the squashed #52 wholesale, restoring the known-good state on all platforms
(the later string-resource and password-sheet commits are untouched). The one
genuinely good idea in there - the network data sources dispatching their own IO for
the future extractable DAV library - can be reintroduced on its own later, with the
ViewModel launches left on ioDispatcher and the KSafe store left exactly as it is.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xt7MRuFpW1jtcHX1MPC4EN
…ub.com/TechbeeAT/spectacled into claude/spectacled-code-review-followup
… apps
Makes the Journals/Notes/Tasks web targets installable PWAs with offline shell
support, using the same webMain/resources serving mechanism DAT-6 already relies on.
Per app:
- manifest.webmanifest: name/short_name, standalone display, start_url/scope '.'
(relative, so it works under the /journals|/notes|/tasks deploy paths), the
variant's brand theme_color (#006896 / #994c2c / #296f23), and a 512x512 icon
copied from the app's existing ic_launcher-playstore.png.
- service-worker.js: network-first for same-origin GETs of static shell assets,
cache only as an offline fallback. Deliberately conservative - it never caches
non-GET, cross-origin, or non-shell requests, so CalDAV/proxy traffic and the
sql.js persistence worker are untouched, and nothing is served stale while
online. Versioned cache, cleaned on activate.
- index.html: <link rel=manifest>, apple-touch-icon, theme-color meta, and a
guarded service-worker registration (no-op where unsupported / on plain HTTP).
BLD-6 was blocked on DAT-6 (web persistence), which has landed. Needs one live
browser pass to verify install + offline reload before calling it done - service
workers, like DAT-6, tend to only reveal issues in a real browser.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xt7MRuFpW1jtcHX1MPC4EN
…ub.com/TechbeeAT/spectacled into claude/spectacled-code-review-followup
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.
… apps
Makes the Journals/Notes/Tasks web targets installable PWAs with offline shell support, using the same webMain/resources serving mechanism DAT-6 already relies on.
Per app:
BLD-6 was blocked on DAT-6 (web persistence), which has landed. Needs one live browser pass to verify install + offline reload before calling it done - service workers, like DAT-6, tend to only reveal issues in a real browser.
Claude-Session: https://claude.ai/code/session_01Xt7MRuFpW1jtcHX1MPC4EN