Design pass: stable nav, two-row plan masthead, labeled toolbar, compact feed - #160
Merged
Conversation
The env badge added text next to the brand, so dev and staging never reproduced production's nav layout — exactly where responsive breakage was going unnoticed. A colored ring on the logo (favicon palette) plus a tooltip carries the same signal without moving anything. Amp-Thread-ID: https://ampcode.com/threads/T-019f8f55-7cc9-73a3-a9cb-1f3e0faa0cb3 Co-authored-by: Amp <amp@ampcode.com>
The eye was a hidden two-click ritual: first click armed a preview pulse, second committed, and wandering off reverted it — which read as "the toggle is broken, it flips back to Private". And the committed state only reached the page via the ActionCable broadcast, so the byline's Private flag routinely went stale until a refresh. Visibility is a two-way switch (PlanPolicy#publish?/#hide?), so there is nothing to guard with a confirmation step. Now: - One click commits. The button repaints optimistically and reverts only if the request fails. - The button is labeled (Private/Shared), not icon-only — it's the one control whose state people need to read at a glance, and the label answers "where do I share this?". - publish/hide answer Turbo Streams: a #plan-header re-render (state flag can't go stale) plus a confirming toast. toast_stream moves from AttachmentsController to the base controller for reuse. - The stale "one-way door" comment on #publish is corrected. Amp-Thread-ID: https://ampcode.com/threads/T-019f8f55-7cc9-73a3-a9cb-1f3e0faa0cb3 Co-authored-by: Amp <amp@ampcode.com>
space-between let the four nav children drift into detached islands at wide widths (Workspace/Feed floating far from the logo), and nothing outside the phone breakpoint was shrink-protected — between ~640 and 900px the gear and bell SVGs crushed to slivers while the bell's absolutely-positioned unread badge stayed put, looking like a red dot with no bell. Now brand + primary links cluster left as one group, search floats in the middle, controls hold the right edge (auto margins split the free space). Icon controls never shrink; the user's name is the designated truncator. A new 900px stage folds the search field into a round icon button before anything else compresses; the 640px phone stage keeps its icons-over-labels treatment. Amp-Thread-ID: https://ampcode.com/threads/T-019f8f55-7cc9-73a3-a9cb-1f3e0faa0cb3 Co-authored-by: Amp <amp@ampcode.com>
The old header jammed the avatar under the title at a flex edge, let the bookmark orphan to the far right on any title wrap, and left uneven dead space when optional chrome was absent. - Title row: type icon pinned to the first title line; the save bookmark slot lives inside the h1 so it trails the final word wherever wrapping puts it. - Meta row: avatar/byline/revision/state flag left, live presence right, indented to the title's text column so the block reads as one unit. - Phone widths drop the indent and let presence wrap under the byline. Verified at 1440/1024/760/390 in light and dark, and that visibility toggles (which Turbo-Stream replace #plan-header) keep the bookmark, state flag, and presence in sync. Amp-Thread-ID: https://ampcode.com/threads/T-019f8f55-7cc9-73a3-a9cb-1f3e0faa0cb3 Co-authored-by: Amp <amp@ampcode.com>
The visibility/edit/archive/history controls floated as loose icons along the document's top edge, reading as scattered chrome — especially when the shelf strip on the left was empty. One translucent pill (existing --glass-* tokens, alpha fallback where backdrop-filter is unsupported) makes them read as a single toolbar; buttons inside go fully round so container and control curves agree. Verified in light/dark at 1440/640/390, with and without shelf chips. Amp-Thread-ID: https://ampcode.com/threads/T-019f8f55-7cc9-73a3-a9cb-1f3e0faa0cb3 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019f8f55-7cc9-73a3-a9cb-1f3e0faa0cb3 Co-authored-by: Amp <amp@ampcode.com>
Three correctness fixes from design review: - Abort the in-flight request on disconnect and refuse to render a stale Turbo Stream after navigation, so a late response can't replace another plan's header. - The header's data-plan-visibility flag is now the authoritative state; the toggle (which lives outside the broadcast-replaced #plan-header) watches header replacements and mirrors it, so an API publish or another tab's toggle keeps the button honest instead of leaving it stuck. - A failed response no longer asserts the old state (the server may have committed before the response was lost) — reload and toast instead, using the same reload-then-toast pattern as folder_dnd. Also: role=toolbar on the plan actions pill, and a system spec covering the external header-replacement sync. Amp-Thread-ID: https://ampcode.com/threads/T-019f8f55-7cc9-73a3-a9cb-1f3e0faa0cb3 Co-authored-by: Amp <amp@ampcode.com>
![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: 2ab04cdc34
ℹ️ 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".
Comment on lines
+49
to
+55
| if (!response.ok) throw new Error("Could not change visibility") | ||
| const data = await response.json() | ||
| this.hiddenValue = data.visibility === "draft" | ||
| } catch { | ||
| // Leave the real state untouched; the reset below repaints it. | ||
| const stream = await response.text() | ||
| // Turbo may have navigated away while the request was in flight — a | ||
| // stream aimed at this page's #plan-header must not land on the next | ||
| // page's header. | ||
| if (!this.element.isConnected) return | ||
| window.Turbo?.renderStreamMessage(stream) |
There was a problem hiding this comment.
When this PATCH is intercepted by auth, such as after the user's session expires, fetch follows the redirect to the HTML sign-in page and the final response can still be ok; this code then feeds non-stream HTML to renderStreamMessage while leaving the optimistically flipped hiddenValue in place. In that case the button reports Shared/Private even though the plan did not change, so verify the response is actually a Turbo Stream (or reject response.redirected) before treating it as success.
Useful? React with 👍 / 👎.
Collaborator
Author
There was a problem hiding this comment.
Resolved by Amp 🤖 — visibility_toggle_controller.js was deleted in the masthead rework (a22a1bd). Visibility changes are now plain button_to forms in the overflow menu, submitted through Turbo's native form handling: no client-side fetch, no optimistic flip, and auth redirects become normal page visits instead of being fed to renderStreamMessage.
This was referenced Jul 23, 2026
Closed
The plan page's top was three loose rows of mixed concerns — title, byline with folder chips, then a strip where actions (archive, eye toggle) sat next to navigation (history) with an unlabeled bookmark jammed into the title. This makes it two rows with clear ownership: - #plan-header (broadcast-replaced) carries only the plan's identity: type icon + title, byline, and rare-state flags (Private/Archived). - The masthead's side column (viewer-relative, request-rendered) holds presence and a glass toolbar: a labeled Edit for people who can edit, a labeled Save/Saved for readers, and one ⋯ menu for everything else — History, Move to folder…, visibility, and Archive (styled destructive, behind a separator). Folder chips and tags leave the reading surface: organizing is a menu action (like Docs' move control), not persistent chrome. The bookmark is gone — Save opens the folder navigator; clicking Saved reopens it with an explicit Remove row, so letting go is deliberate, never an accidental toggle or a scroll jump. Visibility and archive now respond as Turbo Streams that re-render the header, toolbar, and (for archive) a banner slot in place — fixing the production bug where the eye toggle fought the broadcast-replaced header and "Private" stuck around until a refresh. Archiving shows its consequence right where it happened: a banner explaining the plan is hidden from lists, with Restore as the undo. Presence moved outside #plan-header, so plan broadcasts can no longer clobber viewer-relative state (the "(you)" pill survives edits now). The h1's accessible name is the title alone again. Amp-Thread-ID: https://ampcode.com/threads/T-019f8f55-7cc9-73a3-a9cb-1f3e0faa0cb3 Co-authored-by: Amp <amp@ampcode.com>
The feed item's first line was a wrapping flex row, so a long title broke the whole anatomy apart: the type icon orphaned on its own line, "new" stranded below the title, huge gaps everywhere. Now the line is normal text flow — icon, title, and the activity rollup sit inline, so a long title wraps like a sentence with "new" trailing its last word. Items and day sections also tighten up a step, and at phone widths the timestamp stops being a nowrap side column (it was squeezing titles to a word per line at 320) and tucks under the byline instead. Amp-Thread-ID: https://ampcode.com/threads/T-019f8f55-7cc9-73a3-a9cb-1f3e0faa0cb3 Co-authored-by: Amp <amp@ampcode.com>
HamptonMakes
changed the title
The masthead's byline was fake-indented under the icon with a hard-coded padding — history.html.erb never got the indent at all, so its subtitle sat misaligned at the page edge. Both headers are now a single lockup: icon anchored to the title's first line beside a text stack (title over byline) sharing one left edge. Also: history's 280px split now stacks below 720px instead of crushing the diff to a word per line, and the entry count pluralizes (broadcasts include the word so live updates stay in sync). Amp-Thread-ID: https://ampcode.com/threads/T-019f8f55-7cc9-73a3-a9cb-1f3e0faa0cb3 Co-authored-by: Amp <amp@ampcode.com>
The panel is a descendant of the nav, whose own backdrop-filter stops a child backdrop-filter from sampling the page. The 'glass' was just 66% alpha over sharp page text. Dropdowns get a solid elevated surface. Amp-Thread-ID: https://ampcode.com/threads/T-019f8f55-7cc9-73a3-a9cb-1f3e0faa0cb3 Co-authored-by: Amp <amp@ampcode.com>
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
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.
A design-review pass on the plan page, global nav, and Feed, driven by the visual jank visible on real production plans (detached bookmark icons, disappearing bell icon, cramped nav at medium widths, and a visibility toggle that appeared broken). The branch evolved with in-flight feedback: the later commits replace the bookmark-in-title and header visibility toggle from the earlier ones with a two-row masthead and a single labeled toolbar. The final state is what's described here.
What changed (final state)
Environment marker without layout impact (
00855b9)The development badge changed page layout vs production, hiding exactly the bugs this branch fixes. Non-production environments now mark themselves with a colored ring around the logo (+ tooltip) — zero layout delta.
Nav: three stable zones with staged collapse (
3b7f300)At medium widths the nav crushed unpredictably — "Workspace" jammed against the plan title, the bell icon vanished while its red dot floated in space. Now brand + Workspace/Feed cluster left, search holds the center (collapsing to an icon ≤900px), and the right-side controls never shrink. Verified at 1440/1024/900/760/640/390.
Plan masthead: two rows, clear ownership (
a22a1bd)The top of a plan was three loose rows mixing identity, actions, and navigation. Now it's two:
#plan-header, broadcast-replaced): type icon + title, byline, and rare-state flags. Private is a rare state now — it only appears as a flag when it's on.Folder chips and tags left the reading surface entirely — organizing is a menu action (like Docs' move control), not persistent chrome. The bookmark is gone: Save opens the folder navigator, and clicking Saved reopens it with an explicit "Remove from library" row, so letting go is a deliberate choice — never an accidental toggle or a scroll jump.
Visibility and archive: honest, in place (
e42caac→2ab04cd→a22a1bd)The production bug: clicking the eye pulsed and flipped back, and the "Private" byline label went stale until a refresh. Visibility changes now respond as Turbo Streams that re-render the header and toolbar in place — the byline flag and the menu's direction ("Make private" ⇄ "Share with everyone") both track state instantly, with a confirmation toast, no navigation, no scroll.
Archive shows its consequence right where it happened: an in-place banner ("hidden from lists and search, still readable at this URL") with Restore as the undo. The menu drops its Archive entry while archived.
Presence and a11y fixes (
a22a1bd)Presence moved outside the broadcast-replaced
#plan-header, so plan broadcasts can no longer clobber viewer-relative state (fixes #161). The<h1>'s accessible name is the title alone again (fixes #162).Feed: items flow as text (
841c85b)A long title shattered the feed item — icon orphaned on its own line, "new" stranded below. The first line is now normal text flow (icon + title + activity inline), items and day panels tightened a step, and at phone widths the timestamp tucks under the byline instead of squeezing titles to a word per line.
Validation
bundle exec rspec), including rewritten system specs for the menu-based visibility flow, in-place archive/restore, owner Move-to-folder/unfile, and the reader Save → Saved → Remove journey.Screenshots (final state)