add a first-run landing experience - #73
Conversation
morgmart
left a comment
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
REQUEST_CHANGES. Static review of the exact three-dot comparison found six blocking issues and no non-blocking issues. The usage-data control does not govern telemetry; cohort lookup can leave startup blank forever; the narrow welcome layout can clip its only actions without scrolling; fallible filesystem classification can permanently mark an established install as fresh; isolated E2E identifiers always fall back to an unknown cohort; and a post-publication directory-sync error can make the returned cohort disagree with the durable marker. Supplied GitHub evidence was inspected: combined status was pending and no check runs were reported, so required CI still governs merge readiness. No local code, tests, builds, package managers, hooks, scripts, or generated binaries were run.
Deterministic publication result: 6 blocking and 0 non-blocking finding(s) publishable; 0 duplicate(s) suppressed.
morgmart
left a comment
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
REQUEST_CHANGES. A fresh static review of the exact three-dot comparison found four blocking and two non-blocking findings. The previously reported telemetry plumbing and renderer-side never-completes path are substantially improved, but the final flow still treats a preselected checkbox plus the general onboarding CTA as affirmative telemetry consent, and the narrow layout still absolutely positions the actionable section so overflow scrolling cannot reliably recover content above the scroll origin. Cohort classification can also race legacy migration across processes, and an existing non-file database path is permanently classified as fresh rather than unknown. Two additional design-system/accessibility issues remain in the welcome disclosure. Supplied GitHub evidence was inspected: Rust and clean-install checks passed, while Frontend checks and unit tests failed and some jobs were skipped; required CI still governs merge readiness. No local code, tests, builds, package managers, hooks, scripts, or generated binaries were run.
Deterministic publication result: 4 blocking and 2 non-blocking finding(s) publishable; 0 duplicate(s) suppressed.
| <motion.section | ||
| variants={reveal} | ||
| transition={{ duration: reduceMotion ? 0 : 0.35 }} | ||
| className="w-full max-w-[390px] justify-self-start max-[760px]:absolute max-[760px]:top-[42%] max-[760px]:left-1/2 max-[760px]:-translate-x-1/2 max-[760px]:-translate-y-1/2 max-[760px]:text-center" |
There was a problem hiding this comment.
🤖 P1 · Narrow content remains outside scroll flow (blocking)
The latest change restores vertical overflow on the narrow container, but at widths up to 760px the actionable section is still absolutely positioned at top 42% with a negative half-height translation. It contributes no scroll height, and content extending above the scroll origin cannot be reached by scrolling. Short windows, increased text size, zoom, or longer localized copy can therefore still clip the heading or controls.
User effect: At the supported narrow window size or with enlarged text, users can lose access to the welcome heading, primary action, consent control, or disclosure and become stuck during first run.
Recommended fix: Keep the actionable narrow layout in normal document flow with sufficient top and bottom spacing; position only the decorative artifact independently. Validate the minimum window, 200% text/zoom, and Spanish copy.
Test: Add a constrained responsive test at the minimum narrow and short viewport with enlarged text and Spanish localization, and assert the heading, CTA, checkbox, and Learn more control are all reachable by scrolling and operable.
| } else { | ||
| services::app_data_migration::legacy_layout_database_exists(app.handle()) | ||
| }; | ||
| let installation_cohort = |
There was a problem hiding this comment.
🤖 P2 · Cohort classification races migration (blocking)
Startup inspects current and legacy databases and permanently publishes the cohort marker before running legacy migration. Atomic no-clobber publication coordinates marker writers, but no inter-process lock covers the database observations and migration. A concurrently running Berd or legacy Goose Internal process can publish established data after inspection but before migration, allowing this process to persist fresh-with-landing-v1 and then copy/open established data.
User effect: An installation with established data can be permanently treated as new and unexpectedly routed through onboarding because of launch timing between processes.
Recommended fix: Serialize classification and legacy migration with an inter-process startup/migration lock. Under the lock, inspect pre-migration evidence, perform migration, and publish the marker only after the evidence and resulting layout are stable.
Test: Add a lock-boundary or multi-process test that pauses after an absent observation, creates a valid legacy layout from another writer, resumes startup, and asserts the final marker is established rather than fresh.
|
|
||
| pub(crate) fn file_exists(path: &Path) -> Result<bool, String> { | ||
| match fs::metadata(path) { | ||
| Ok(metadata) => Ok(metadata.is_file()), |
There was a problem hiding this comment.
🤖 P2 · Ambiguous database paths become fresh (blocking)
file_exists returns false for every existing path whose metadata is not a regular file. A directory, FIFO, or other unexpected entry at either database path is therefore treated exactly like NotFound, and startup may permanently publish the fresh cohort even though layout initialization or migration cannot safely use that path. This bypasses the otherwise fail-closed metadata-error behavior.
User effect: After a filesystem conflict or malformed installation path, Berd can save the wrong permanent cohort and later force an established user through onboarding once the path problem is repaired.
Recommended fix: Return an error for an existing non-file path and explicitly define symlink handling, so ambiguous evidence resolves to unknown without publishing a marker.
Test: Create a directory at the current and legacy database paths and assert detection errors and no cohort marker is written; cover the intended symlink policy separately.
| <DialogContent | ||
| size="xl" | ||
| closeLabel={t("actions.close", { ns: "common" })} | ||
| className="gap-5 rounded-[28px] p-10 pr-12" |
There was a problem hiding this comment.
🤖 P2 · Welcome dialog bypasses shared chrome (non-blocking)
The disclosure applies feature-local radius, padding, scrim color, and backdrop filters directly to DialogContent. Project guidance assigns modal surface geometry and visual behavior to the shared Dialog API, so this creates a second recipe that can drift across themes and future design-system changes.
User effect: The first-run disclosure can look and behave inconsistently with other Berd dialogs as the shared system evolves.
Recommended fix: Use the existing Dialog surface defaults, or add a named shared semantic surface/overlay option with documented theme mappings and explorer coverage.
Test: Cover the semantic option in the design-system explorer or component tests and assert WelcomeStep selects it without feature-owned surface color, radius, or backdrop classes.
| id={checkboxId} | ||
| checked={shareUsageData} | ||
| onCheckedChange={(checked) => setShareUsageData(checked === true)} | ||
| aria-describedby={`${checkboxId}-description`} |
There was a problem hiding this comment.
🤖 P2 · Checkbox description repeats controls (non-blocking)
The checkbox gets its accessible name from the associated label, then aria-describedby points to the enclosing paragraph containing that same label and the separate Learn more button. Assistive technology can announce the consent sentence twice and include the neighboring button label as checkbox description.
User effect: Screen-reader users may hear a confusing duplicated consent announcement that blends the checkbox with a separate action.
Recommended fix: Remove aria-describedby unless there is distinct supporting copy. Keep the checkbox associated only with its label and leave Learn more as an independent adjacent control.
Test: Add an accessibility assertion for the checkbox's computed name and description, verifying the sentence is announced once and Learn more is exposed only as its own button.
The telemetry consent gate added to submitFeedbackReport skips trackFeedbackSubmitted unless berd:telemetry-consent:v1 is stored, so FeedbackDialog's "tracks feedback submitted after the backend accepts it" case failed with an unseeded localStorage. Seed the consent key in beforeEach, matching submitFeedbackReport.test.ts. Signed-off-by: Matt Toohey <contact@matttoohey.com>
morgmart
left a comment
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
REQUEST_CHANGES. A fresh static review of the exact three-dot comparison found two new blocking and two new non-blocking findings. Telemetry implementation exceptions can prevent onboarding progression or leave later launches stuck on startup, and cohort uncertainty still silently routes established users into first-run onboarding. The new startup loader is mounted while the native window remains hidden, and the shared dialog now owns a one-off inline close icon outside the established icon system. Six still-present findings were suppressed because their prior automated threads remain unresolved. The final self-check covered the first-run and established-install flows, telemetry opt-in/out and feedback tracking, startup success/error/timeout/never-completes behavior, migration and concurrent-writer races, responsive/reflow behavior, accessibility, i18n, navigation gates, test honesty, design-system rules, and duplicate overlap. Supplied GitHub evidence was inspected: DCO, clean-room install, and completed Rust checks passed while frontend and Windows checks were in progress; required CI still governs merge readiness. No local code, tests, builds, package managers, hooks, scripts, or generated binaries were run.
Deterministic publication result: 2 blocking and 2 non-blocking finding(s) publishable; 6 duplicate(s) suppressed.
| export function startTelemetryIfConsented(): boolean { | ||
| if (started || getTelemetryConsent() !== true) return false; | ||
| started = true; | ||
| initTelemetry(); |
There was a problem hiding this comment.
🤖 P1 · Telemetry failures block core flows (blocking)
startTelemetryIfConsented calls the distribution-provided telemetry implementation without containing exceptions. On the welcome screen, an exception prevents onStart from running after consent has already been persisted. On subsequent launches, the same helper runs inside the cohort promise's finally callback before the main React tree is rendered, so an exception leaves the application on StartupLoadingView. Because started is set before initialization, the current renderer lifecycle cannot retry either.
User effect: A telemetry SDK failure can make the “Let’s go” action stop working or prevent Berd from opening on every later launch for a user who opted in.
Recommended fix: Make telemetry strictly best-effort inside the shared startup owner: catch and report initialization and launch-event failures, preserve onboarding and rendering in a finally-safe path, and define retry/idempotence behavior without allowing telemetry to own application availability.
Test: Mock initTelemetry and trackAppLaunched to throw separately and assert the welcome action still calls onStart, main startup still renders App, the error is reported, and repeated attempts follow the intended retry/idempotence policy.
startTelemetryIfConsented called into the client seam with no error containment. The seam is empty in this tree but overlaid by the internal distribution at build time, where init can do network setup, storage access, or config validation. A throw stranded both call sites: in main.tsx the helper ran inside the cohort promise's .finally() just before reactRoot.render, so the app stayed on StartupLoadingView forever; in WelcomeStep consent was already persisted to localStorage, so the click wrote consent but never advanced onboarding. Wrap each seam call and report through reportRendererError, which applies the existing secret redaction. Init and the launch event get separate policies: a failed init returns false and disables telemetry until relaunch rather than retrying into a broken SDK, while a failed launch event leaves telemetry running since init already succeeded. Keeping the started latch set before the attempt bounds failures to one per renderer process. Also make the two call sites independent of the helper's behavior, so a future regression cannot block boot or onboarding: move the call after reactRoot.render in main.tsx, and guarantee onStart() from a finally in WelcomeStep. Signed-off-by: Matt Toohey <contact@matttoohey.com>
morgmart
left a comment
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
REQUEST_CHANGES. A fresh static review of the exact three-dot comparison found four blocking and two non-blocking publishable findings. The telemetry failure-containment change fixes the prior core-flow failure, but telemetry consent still defaults on through the general progression action and has no app-wide lifecycle for later opt-in or revocation. Cohort aggregation can discard definitive established-install evidence when the other probe fails, while unknown classification still silently enters first-run onboarding. The startup loader remains mounted behind an initially hidden native window, and the shared dialog still forks the icon system. Five still-present issues were suppressed because their prior automated threads remain unresolved; the prior telemetry-failure issue was verified fixed. The final self-check covered fresh and established onboarding flows, consent and revocation, feedback telemetry, startup success/error/timeout/visibility, migration and concurrent writers, responsive/reflow behavior, accessibility, i18n, navigation gates, tests, design-system rules, and duplicate overlap. Supplied GitHub evidence was inspected: all eight reported checks completed successfully. No local code, tests, builds, package managers, hooks, scripts, or generated binaries were run.
Deterministic publication result: 4 blocking and 2 non-blocking finding(s) publishable; 5 duplicate(s) suppressed.
| const checkboxId = useId(); | ||
| const headingRef = useRef<HTMLHeadingElement>(null); | ||
| const [shareUsageData, setShareUsageData] = useState( | ||
| () => getTelemetryConsent() ?? true, |
There was a problem hiding this comment.
🤖 P1 · Progression still implies telemetry consent (blocking)
When no preference exists, the sharing checkbox initializes checked. Clicking the general “Let’s go” onboarding action persists that untouched default and starts telemetry. The system now respects the stored value, but it still derives affirmative consent from proceeding through onboarding rather than from a distinct positive sharing choice. The earlier automated thread was resolved without a substantive human reply, and the issue remains in the current comparison, so it is eligible to be reopened.
User effect: A user who simply continues setup begins sharing usage data without distinctly opting in, despite the screen presenting sharing as a choice.
Recommended fix: Fail closed when consent is unset: initialize the control unchecked, or require a separate explicit affirmative sharing action. Keep the onboarding progression action independent from telemetry consent.
Test: Start with no stored preference, click “Let’s go” without interacting with sharing, and assert telemetry remains disabled; then explicitly opt in and assert the durable preference and one-time startup behavior.
| // Consent is persisted before telemetry starts, so onboarding has | ||
| // to advance regardless of how the telemetry seam behaves. | ||
| try { | ||
| if (setTelemetryConsent(shareUsageData) && shareUsageData) { |
There was a problem hiding this comment.
🤖 P1 · Consent cannot be changed later (blocking)
WelcomeStep is the only production surface that writes the new app-wide telemetry preference. Startup and feedback treat it as authoritative, but there is no persistent settings surface or shared lifecycle for changing it after onboarding, and the process-level started latch has no disable path. Fresh users who opt in cannot revoke consent; established users are graduated past the only choice surface and cannot opt in. Onboarding has therefore become the accidental owner of a system-wide privacy lifecycle.
User effect: Users cannot later withdraw data-sharing consent, and existing users who skipped onboarding cannot express a preference at all.
Recommended fix: Give telemetry an app-wide consent owner that reads and updates the preference, starts collection when enabled, stops or disables it when revoked, and notifies consumers. Use that owner from both onboarding and a persistent Privacy/Telemetry setting available to every user.
Test: Cover the full lifecycle: established user opts in from settings, fresh user revokes after onboarding, telemetry stops or disables according to the SDK contract, feedback tracking follows the live preference, and the choice persists across restart.
| return Ok(record.cohort); | ||
| } | ||
|
|
||
| let current_layout_exists = current_layout_exists?; |
There was a problem hiding this comment.
🤖 P2 · Definitive established evidence is discarded (blocking)
The current and legacy database probe Results are unwrapped sequentially. If either probe is Ok(true) but the other probe errors, initialization returns Unknown instead of using the already definitive evidence that an established database exists. For example, a valid current berd.sqlite is ignored when permissions prevent inspecting the obsolete legacy directory.
User effect: An established user can be sent into first-run onboarding because an irrelevant secondary filesystem probe failed, even though Berd already found their existing database.
Recommended fix: Aggregate the probes as three-state evidence: classify established if either is Ok(true), classify fresh only when both are Ok(false), and return unknown/error only when no positive evidence exists and at least one probe is uncertain.
Test: Add cases for Ok(true) plus Err and Err plus Ok(true), asserting both classify and persist established; retain both-false and uncertainty-without-positive-evidence cases.
| .catch((error) => { | ||
| console.error("Failed to resolve installation cohort:", error); | ||
| reportRendererError("installation_cohort_failed", error); | ||
| initializeOnboardingGraduation("unknown"); |
There was a problem hiding this comment.
🤖 P2 · Unknown cohort still forces onboarding (blocking)
Metadata errors, unreadable or unsupported markers, backend initialization failures, and renderer timeout all resolve to Unknown. Graduation leaves onboarding pending for Unknown, while AppShell shows onboarding for every lifecycle except completed. This converts “classification unavailable” into “definitely fresh.” The prior automated thread was resolved without a substantive human reply, and the behavior remains, so it is eligible to be reopened.
User effect: A transient disk, permissions, marker, or timeout problem can force an established user through first-run provider and agent setup, potentially on every launch.
Recommended fix: Keep unknown distinct from fresh. Use a retryable recovery state or conservatively bypass onboarding for unknown so first-run UI appears only after affirmative fresh classification.
Test: Exercise timeout, metadata failure, malformed marker, and unsupported marker through the main-to-store-to-AppShell boundary and assert the chosen recovery behavior does not silently show fresh-install onboarding to an established profile.
| </QueryClientProvider> | ||
| </RendererErrorBoundary> | ||
| </TooltipProvider> | ||
| <I18nProvider> |
There was a problem hiding this comment.
🤖 P2 · Startup loader remains hidden (non-blocking)
Bootstrap mounts StartupLoadingView before cohort resolution, but the main Tauri window is configured initially invisible. Window.show remains in App's effect, and App is not mounted until the cohort request settles or times out. The loading surface therefore cannot provide visible feedback during that wait. The prior automated thread was resolved without a substantive human reply, and the behavior remains, so it is eligible to be reopened.
User effect: During a slow cohort lookup, users see no Berd window for up to five seconds and then see it appear abruptly instead of seeing startup progress.
Recommended fix: Make bootstrap own initial window visibility and show the main window immediately after mounting the loading surface, while preserving normal App startup and diagnostic behavior.
Test: Hold the cohort promise pending and spy on the Tauri window API, asserting the loading surface is mounted and the main window is shown before resolution or timeout.
| import * as DialogPrimitive from "@radix-ui/react-dialog"; | ||
| import { Slot } from "@radix-ui/react-slot"; | ||
| import { XIcon } from "lucide-react"; | ||
| function SolidXIcon({ className }: { className?: string }) { |
There was a problem hiding this comment.
🤖 P3 · Dialog still forks icon system (non-blocking)
The shared Dialog primitive replaces the established icon-library close affordance with a private inline filled SVG. This changes every dialog globally and creates an undocumented one-off icon recipe rather than using or extending the shared icon system. The prior automated thread was resolved without a substantive human reply, and the implementation remains, so it is eligible to be reopened.
User effect: Every dialog receives a close affordance that can drift in weight, sizing, and future treatment from the rest of Berd's icons.
Recommended fix: Use the established shared icon-family component, or introduce and document a reusable system-owned close icon rather than embedding a private SVG in Dialog.
Test: Add shared Dialog explorer or visual coverage asserting the approved close icon primitive at supported sizes and themes.
3 participants
Category: new-feature
User Impact: New Berd installations now open with a responsive welcome experience before entering the main canvas.
Problem: Berd previously entered the product without a designed first-run introduction or clear usage-data disclosure. Existing installations also need to remain uninterrupted when the experience becomes the default.
Solution: Add an interactive, accessible landing page and graduate the old experiment into the normal first-run flow, backed by a durable installation-cohort marker that safely distinguishes new installs from upgrades.
Note
The usage-data checkbox is intentionally awaiting the follow-up telemetry preference hookup and should not be treated as release-ready consent behavior yet.
File changes
src-tauri/Cargo.toml
Makes unique temporary-file support available to the installation cohort service.
src-tauri/src/commands/installation.rs
Exposes the resolved installation cohort to the renderer after startup initialization settles.
src-tauri/src/commands/mod.rs
Registers the installation command module.
src-tauri/src/lib.rs
Initializes cohort state before app-data migration and registers the command without blocking startup on classification failures.
src-tauri/src/services/app_data_migration.rs
Provides authoritative legacy-layout detection for upgrade classification.
src-tauri/src/services/installation_cohort.rs
Persists a versioned fresh-versus-established cohort marker with concurrent-launch and crash-safety handling.
src-tauri/src/services/mod.rs
Registers the installation cohort service.
src/app/AppShell.tsx
Makes incomplete onboarding the normal first-run route instead of an experimental route.
src/features/experiments/ExperimentsSettings.tsx
Removes obsolete hidden-experiment filtering after first-run onboarding graduation.
src/features/experiments/tests/ExperimentsSettings.test.tsx
Updates experiment settings coverage for the graduated flow.
src/features/experiments/experimentDefinitions.ts
Removes the retired first-run onboarding experiment definition.
src/features/onboarding/api/installationCohort.ts
Adds the typed renderer API for reading the native installation cohort.
src/features/onboarding/model/onboardingStore.test.ts
Covers fresh installs, upgrades, malformed records, unavailable storage, and persistence compatibility.
src/features/onboarding/model/onboardingStore.ts
Graduates established users without showing onboarding while preserving authored or newer-version state.
src/features/onboarding/ui/OnboardingShell.tsx
Allows the welcome layout to customize its content overflow without forking the shell.
src/features/onboarding/ui/WelcomeStep.test.tsx
Covers welcome actions, focus, reduced motion, project rendering, and disclosure behavior.
src/features/onboarding/ui/WelcomeStep.tsx
Implements the responsive landing page, interactive project cube, consent disclosure, and glass dialog.
src/features/projects/artifact/ProjectArtifactPreview.tsx
Passes optional camera framing through the artifact preview boundary.
src/features/projects/artifact/ProjectArtifactRenderer.tsx
Supports adjusted camera distance for oversized embedded project cubes.
src/features/projects/artifact/types.ts
Adds the typed camera-distance renderer contract.
src/main.test.tsx
Verifies cohort lookup and startup fallback behavior for main and session windows.
src/main.tsx
Resolves installation cohort before rendering the main app while preserving session-window startup.
src/shared/i18n/locales/en/onboarding.json
Adds English welcome and usage-data disclosure copy.
src/shared/i18n/locales/en/settings.json
Removes retired experiment copy.
src/shared/i18n/locales/es/onboarding.json
Adds Spanish welcome and usage-data disclosure copy.
src/shared/i18n/locales/es/settings.json
Removes retired experiment copy.
src/shared/ui/dialog.tsx
Uses a shared solid close icon and supports a localized accessible close label.