Show first-run onboarding hint in terminal and persist its state #177
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8544b7da3e
ℹ️ 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".
| const show_onboarding = !persistence.onboarding_shown; | ||
| if (show_onboarding) { | ||
| persistence.onboarding_shown = true; | ||
| persistence.save(allocator) catch |err| { | ||
| log.warn("Failed to save onboarding state: {}", .{err}); |
There was a problem hiding this comment.
The flag is persisted as soon as show_onboarding is computed, before any frame is drawn. However the hint itself only renders when the focused session is slot 0, alive, and there’s enough visible rows (see the early-return guard in renderOnboardingHint). If the app starts in a small window or the hint is otherwise skipped on the first run, the persisted onboarding_shown = true will suppress the hint permanently, even after resizing/relaunching. Consider deferring the save until after a successful render (or tracking that the hint was actually displayed).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR adds a first-run onboarding hint to help users discover core keyboard shortcuts and the architect hook command. The hint appears once per installation and its state is persisted to prevent repeated displays.
Changes:
- Added
onboarding_shownflag to persistence configuration with TOML serialization support - Implemented in-terminal rendering of onboarding hint for slot 0 when focused and alive
- Updated documentation to describe the new onboarding feature and persistence field
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/config.zig | Added onboarding_shown field to Persistence struct with TOML load/save logic and test coverage |
| src/app/runtime.zig | Added logic to check and set onboarding flag on startup, passing state to renderer |
| src/render/renderer.zig | Threaded show_onboarding parameter through rendering pipeline and implemented renderOnboardingHint function |
| docs/configuration.md | Documented new onboarding_shown persistence field |
| docs/architecture.md | Added mention of first-run onboarding hint in rendering features list |
| README.md | Added user-facing description of onboarding hint behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) RenderError!void { | ||
| const lines = [_][]const u8{ | ||
| "Welcome to Architect", | ||
| "Cmd+N adds a terminal, Cmd+W closes one.", |
There was a problem hiding this comment.
Corrected spelling of 'recieve' to 'receive'.
| try flushRun(font, run_buf[0..], run_len, run_x, origin_y + @as(c_int, @intCast(row)) * cell_height_actual, run_cells, cell_width_actual, cell_height_actual, run_fg, run_variant); | ||
| } | ||
|
|
||
| if (show_onboarding and is_focused and session.slot_index == 0 and !session.dead) { |
There was a problem hiding this comment.
The compound boolean condition checks multiple unrelated properties. Consider extracting this into a helper function shouldShowOnboarding that takes these parameters and returns a boolean, improving readability and testability.
| defer result.deinit(); | ||
| persistence.window = result.value.window; | ||
| persistence.font_size = result.value.font_size; | ||
| persistence.onboarding_shown = result.value.onboarding_shown orelse true; |
There was a problem hiding this comment.
When loading v2 configuration with missing onboarding_shown field, defaulting to true assumes the onboarding has been shown. This will prevent first-time users upgrading from an older version without this field from ever seeing the onboarding hint. The default should be false to ensure users see the hint unless explicitly marked as shown.
| persistence.onboarding_shown = result.value.onboarding_shown orelse true; | |
| persistence.onboarding_shown = result.value.onboarding_shown orelse false; |
|
|
||
| loaded.window = result.value.window; | ||
| loaded.font_size = result.value.font_size; | ||
| loaded.onboarding_shown = result.value.onboarding_shown orelse true; |
There was a problem hiding this comment.
Same issue as in the main loading logic: defaulting to true when the field is missing will cause the test to incorrectly validate behavior for missing fields. Should default to false for consistency with the expected behavior of showing onboarding to new users.
| loaded.onboarding_shown = result.value.onboarding_shown orelse true; | |
| loaded.onboarding_shown = result.value.onboarding_shown orelse false; |
Motivation
architecthook command.Description
onboarding_shownflag toPersistenceand include it in TOML load/save semantics (src/config.zig).show_onboarding = !persistence.onboarding_shown, mark the flag true and save immediately when the hint will be shown (src/app/runtime.zig).show_onboardinginto the renderer and render a dim, in-terminal hint (slot 0, focused and alive) with the lines describingCmd+N,Cmd+W,Cmd+Enterandarchitect hook ...(src/render/renderer.zig).README.md,docs/configuration.md,docs/architecture.md) and add a persistence round-trip expectation to tests insrc/config.zig.Testing
zig buildandzig build test, butzigwas not available in the environment so builds/tests could not be executed (command not found).just lintandzig fmtbut the required tools were not installed so lint/format checks could not be run (command not found).Persistencewere updated but not run due to the missingzigtoolchain.Codex Task