feat(app): replace release notes dialog with subtle toast (#486)#488
feat(app): replace release notes dialog with subtle toast (#486)#488
Conversation
The subtle variant uses surface-raised + fg-strong + border-weak for non-urgent informational notices, distinct from the default deep tone reserved for action receipts and errors. pre-line whitespace preserves multi-line content. The onDismiss option fires once when the toast root unmounts via solid-js onCleanup, covering close button, action click, and programmatic dismiss without leaking Kobalte lifecycle internals to consumers.
Reshape parser to ReleaseSummary { tag, description, localeUsed }; drop
the structured-highlights branch (Array.isArray(value.highlights), the
value.releases wrapper accessor, parseHighlight/parseMedia/Highlight.media,
MAX_STRUCTURED_HIGHLIGHTS) which has never been used in any release.
Switch the controller from dialog.show to showToast with the new subtle
variant. markSeen moves from show-time to dismiss-time via onDismiss so
close button, action click, and programmatic dismiss all converge on
one hook.
For locale fallback, ReleaseSummary carries the locale the parser ended
up using; when the user-locale section is missing the toast title and
action copy fall back together with the description, avoiding mixed-
language output. The localized copy lives in TOAST_COPY since
@solid-primitives/i18n has no per-call locale override.
When the current platform version is not in the changelog (dev builds,
custom tags) sliceHighlights returns empty rather than slicing from
index 0 and surfacing every release. Multi-version merges keep the
newest segment description-only (its tag is in the title) and prefix
subsequent segments with their tag. Cap stays at 5 versions; older
skipped releases remain accessible via the GitHub link.
The release-notes dialog is fully replaced by the subtle toast in highlights.tsx. Nothing else imports DialogReleaseNotes or the file itself.
The dialog.releaseNotes.* keys (getStarted, next, hideFuture, media.alt)
served the deleted DialogReleaseNotes component and have no consumers
left. Replace with toast.releaseNotes.{title,action.viewFull} for
future consumers that want locale-following copy. Today's release-notes
toast goes through TOAST_COPY in highlights.tsx so it can honor the
parser's effective locale on fallback; the i18n strings happen to match
the en/zh values in TOAST_COPY.
The toast.releaseNotes.* keys added in the previous commit were never read: TOAST_COPY in highlights.tsx owns the title and action strings because the parsed body's locale (not the user's UI locale) drives the copy when GitHub releases lack the user's locale section — the @solid-primitives/i18n helper has no per-call locale override, so a small inline copy table is the cleanest path. Document the rationale inline so future maintainers don't restore the keys reflexively. Also add a one-line comment on toast.tsx onCleanup explaining that it covers all dismiss paths (close button, action click, swipe, programmatic), which is the contract consumers rely on.
Six Playwright tests cover the user-visible toast path: subtle variant appears with title/description/icon/action when the stored version is older than current; close button writes markSeen to localStorage; releaseNotes=false suppresses the toast; multi-version skipped releases merge with version labels; locale fallback keeps title and action in sync with the parsed body locale (en when zh section is missing); and zh title/action render when a zh release section is present. Mocks the GitHub Releases API via page.route and seeds highlights.v1 plus pawwork.global.dat:language via addInitScript so the controller's update path runs deterministically without network.
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces the blocking release-notes modal with a persistent locale-aware subtle toast that merges up to five skipped releases, introduces a ReleaseSummary model and toast onDismiss lifecycle, updates parsing to prefer release-body notices with locale detection, removes the DialogReleaseNotes component, and adds unit and E2E tests plus subtle toast styles. ChangesRelease Notes Toast Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces the release notes dialog with a subtle toast notification and simplifies the highlights parsing logic by removing support for structured highlights. It introduces a new 'subtle' toast variant and includes comprehensive E2E tests. The review feedback correctly identifies a TypeScript compilation error caused by a type mismatch between the application's full locale union and the subset supported by the release notes parser; suggestions were provided to narrow the internal locale type and update function signatures accordingly.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/ui/src/components/toast.tsx (1)
121-133: 💤 Low valueClarify that
onCleanupalso fires on parent unmount, not only dismiss paths.The inline comment lists "close button, action click, swipe, programmatic
toaster.dismiss" butonCleanupwill also run when the enclosing reactive owner (i.e., the<Toast.Region>/ app root) unmounts. Callers wiringonDismissto side effects with "user-acknowledged" semantics (e.g.,markSeeninhighlights.tsx) will get the callback on app teardown too. Worth softening the comment so future readers don't treatonDismissas strictly user-initiated. I'll raise the actual semantic risk on the call site.📝 Proposed comment update
- // onCleanup runs when the toast root unmounts (close button, action click, - // swipe, programmatic toaster.dismiss) — covering all dismiss paths in one - // hook. The dismissed guard is defense against future Kobalte upgrades - // re-invoking this render closure. + // onCleanup runs whenever the toast root unmounts: explicit dismiss paths + // (close button, action click, swipe, programmatic toaster.dismiss) AND + // ambient unmounts (parent owner teardown, e.g., app exit). Callers that + // require "user-acknowledged" semantics should not rely on this firing + // only after a deliberate dismiss. The `dismissed` guard is defense + // against future Kobalte upgrades re-invoking this render closure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/toast.tsx` around lines 121 - 133, The inline comment inside the toaster.show render closure (around the onCleanup usage that guards opts.onDismiss via the dismissed flag) incorrectly implies onCleanup only runs for user-dismiss paths; update the comment near toaster.show/onCleanup/opts.onDismiss to explicitly mention that onCleanup also fires when the enclosing reactive owner (e.g., <Toast.Region> or app root) unmounts, and soften the language so callers know onDismiss may run on parent/unmount as well (preserving the dismissed guard logic with the dismissed variable unchanged).packages/app/src/context/highlights.tsx (1)
294-314: 💤 Low valueDocument the 500ms
setTimeoutrationale (or replace with an explicit readiness signal).The deferred
showToasthas no comment explaining why it waits 500ms — readers are left to guess (waiting for<Toast.Region>mount? avoiding splash overlap?). A brief inline note, or better, awaiting an explicit signal (e.g., the toast region's mount event), prevents this from looking like a flake-mitigation hack.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/context/highlights.tsx` around lines 294 - 314, The 500ms setTimeout wrapping showToast (where timer is assigned, using summaries[0], TOAST_COPY, buildToastDescription, platform.openLink, and markSeen) is undocumented and fragile; either add a concise inline comment explaining the exact reason for the delay (e.g., waiting for <Toast.Region> mount or avoiding UI overlap) or, preferably, replace the magic delay with an explicit readiness signal: introduce/await a toastRegionReady promise or subscribe to a toastRegionMounted event before calling showToast, then remove the setTimeout and the timer variable to ensure deterministic behavior.packages/app/e2e/release-notes/release-notes-toast.spec.ts (1)
6-54: 💤 Low valueConstants should use
SCREAMING_SNAKE_CASEper the e2e guidelines.
singleReleasePayload,multiVersionPayload,localizedPayload, and the varioustoast*Selectorconstants are file-level immutable values, which the project convention treats as constants.RELEASES_URL_PATTERNandHIGHLIGHTS_KEYalready follow the convention; the rest should match.As per coding guidelines: "Use SCREAMING_SNAKE_CASE for constants in tests".
🛠️ Proposed renames (apply consistently to all references)
-const singleReleasePayload = [ +const SINGLE_RELEASE_PAYLOAD = [ … ] -const multiVersionPayload = [ +const MULTI_VERSION_PAYLOAD = [ … ] -const localizedPayload = [ +const LOCALIZED_PAYLOAD = [ … ] -const toastSelector = '[data-component="toast"][data-variant="subtle"]' -const toastTitleSelector = `${toastSelector} [data-slot="toast-title"]` -const toastDescriptionSelector = `${toastSelector} [data-slot="toast-description"]` -const toastActionSelector = `${toastSelector} [data-slot="toast-action"]` -const toastCloseButtonSelector = `${toastSelector} [data-slot="toast-close-button"]` -const toastIconSelector = `${toastSelector} [data-slot="toast-icon"]` +const TOAST_SELECTOR = '[data-component="toast"][data-variant="subtle"]' +const TOAST_TITLE_SELECTOR = `${TOAST_SELECTOR} [data-slot="toast-title"]` +const TOAST_DESCRIPTION_SELECTOR = `${TOAST_SELECTOR} [data-slot="toast-description"]` +const TOAST_ACTION_SELECTOR = `${TOAST_SELECTOR} [data-slot="toast-action"]` +const TOAST_CLOSE_BUTTON_SELECTOR = `${TOAST_SELECTOR} [data-slot="toast-close-button"]` +const TOAST_ICON_SELECTOR = `${TOAST_SELECTOR} [data-slot="toast-icon"]`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/e2e/release-notes/release-notes-toast.spec.ts` around lines 6 - 54, The file-level immutable test constants singleReleasePayload, multiVersionPayload, localizedPayload and the selector constants toastSelector, toastTitleSelector, toastDescriptionSelector, toastActionSelector, toastCloseButtonSelector, toastIconSelector should be renamed to SCREAMING_SNAKE_CASE (e.g., SINGLE_RELEASE_PAYLOAD, MULTI_VERSION_PAYLOAD, LOCALIZED_PAYLOAD, TOAST_SELECTOR, TOAST_TITLE_SELECTOR, TOAST_DESCRIPTION_SELECTOR, TOAST_ACTION_SELECTOR, TOAST_CLOSE_BUTTON_SELECTOR, TOAST_ICON_SELECTOR) and all usages in release-notes-toast.spec.ts updated accordingly; keep RELEASES_URL_PATTERN and HIGHLIGHTS_KEY as-is since they already follow the convention. Ensure only the identifier names change (no runtime behavior) and update any imports/references within the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/app/e2e/release-notes/release-notes-toast.spec.ts`:
- Around line 127-137: Replace the wall-clock wait in the test by polling
localStorage until the highlights version advances: instead of awaiting
page.waitForTimeout(1500), call expect.poll on a page.evaluate that reads
localStorage[HIGHLIGHTS_KEY] (the same key used by markSeen()/start()) and
returns the parsed version, and poll until it equals "2026.5.7"; once the poll
succeeds, assert page.locator(toastSelector) hasCount(0). This removes the flaky
sleep and uses the deterministic readiness signal produced when
start()/markSeen() runs with releaseNotes false.
- Around line 97-105: Replace the one-shot localStorage read with a polled
assertion using expect.poll so the test waits for the persistence write to
complete after the toast is closed; specifically, after clicking
toastCloseButtonSelector and awaiting toastSelector toBeHidden(), use
expect.poll to repeatedly evaluate localStorage.getItem(HIGHLIGHTS_KEY) (parse
JSON) and assert the stored?.version equals "2026.5.7" instead of calling
page.evaluate once — this ensures the markSeen/setStore persistence has finished
before asserting.
In `@packages/app/src/context/highlights.tsx`:
- Around line 294-314: The toast onDismiss is being called during unmount and
incorrectly marks releases seen; change the showToast usage in the highlights
code (the setTimeout block that calls showToast) to provide a user-initiated
guard: create a local boolean (e.g., userInitiated) scoped to that toast, set it
true only from user actions (the action onClick for viewFull and the toast
close-button handler in toast.tsx), and replace direct onDismiss: markSeen with
an onDismiss that only calls markSeen if userInitiated is true; update the toast
component’s close-button handler (and any explicit close-path) to flip that flag
so only explicit user dismissals or action clicks call markSeen while
unmount/cleanup still runs without marking versions seen.
---
Nitpick comments:
In `@packages/app/e2e/release-notes/release-notes-toast.spec.ts`:
- Around line 6-54: The file-level immutable test constants
singleReleasePayload, multiVersionPayload, localizedPayload and the selector
constants toastSelector, toastTitleSelector, toastDescriptionSelector,
toastActionSelector, toastCloseButtonSelector, toastIconSelector should be
renamed to SCREAMING_SNAKE_CASE (e.g., SINGLE_RELEASE_PAYLOAD,
MULTI_VERSION_PAYLOAD, LOCALIZED_PAYLOAD, TOAST_SELECTOR, TOAST_TITLE_SELECTOR,
TOAST_DESCRIPTION_SELECTOR, TOAST_ACTION_SELECTOR, TOAST_CLOSE_BUTTON_SELECTOR,
TOAST_ICON_SELECTOR) and all usages in release-notes-toast.spec.ts updated
accordingly; keep RELEASES_URL_PATTERN and HIGHLIGHTS_KEY as-is since they
already follow the convention. Ensure only the identifier names change (no
runtime behavior) and update any imports/references within the file.
In `@packages/app/src/context/highlights.tsx`:
- Around line 294-314: The 500ms setTimeout wrapping showToast (where timer is
assigned, using summaries[0], TOAST_COPY, buildToastDescription,
platform.openLink, and markSeen) is undocumented and fragile; either add a
concise inline comment explaining the exact reason for the delay (e.g., waiting
for <Toast.Region> mount or avoiding UI overlap) or, preferably, replace the
magic delay with an explicit readiness signal: introduce/await a
toastRegionReady promise or subscribe to a toastRegionMounted event before
calling showToast, then remove the setTimeout and the timer variable to ensure
deterministic behavior.
In `@packages/ui/src/components/toast.tsx`:
- Around line 121-133: The inline comment inside the toaster.show render closure
(around the onCleanup usage that guards opts.onDismiss via the dismissed flag)
incorrectly implies onCleanup only runs for user-dismiss paths; update the
comment near toaster.show/onCleanup/opts.onDismiss to explicitly mention that
onCleanup also fires when the enclosing reactive owner (e.g., <Toast.Region> or
app root) unmounts, and soften the language so callers know onDismiss may run on
parent/unmount as well (preserving the dismissed guard logic with the dismissed
variable unchanged).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 701252ed-5cc6-444e-8c3d-9295cdc115d6
📒 Files selected for processing (8)
packages/app/e2e/release-notes/release-notes-toast.spec.tspackages/app/src/components/dialog-release-notes.tsxpackages/app/src/context/highlights.test.tspackages/app/src/context/highlights.tsxpackages/app/src/i18n/en.tspackages/app/src/i18n/zh.tspackages/ui/src/components/toast.csspackages/ui/src/components/toast.tsx
💤 Files with no reviewable changes (3)
- packages/app/src/i18n/zh.ts
- packages/app/src/components/dialog-release-notes.tsx
- packages/app/src/i18n/en.ts
onCleanup also fires on ambient unmount (parent owner teardown / app exit), so wiring `onDismiss: markSeen` directly silently marked unseen releases as seen on app close. Spec #486 requires "quitting unread re-shows on next launch", so onDismiss must distinguish user-initiated dismissal from teardown. Add a userDismissed flag set only by close-button click and action-button click handlers. onCleanup gates the onDismiss callback behind that flag — ambient unmounts no longer fire onDismiss. Update inline comment to reflect the dual unmount semantics; document the 500ms toast defer rationale in highlights.tsx. Reported by CodeRabbit AI on PR #488.
Two race-prone patterns were violating packages/app/e2e/AGENTS.md "Wait on state": page.waitForTimeout(1500) as the suppression signal, and a one-shot localStorage read after the toast hides. Both can read stale state because the persistence write and the visibility transition are not strictly ordered. Replace with expect.poll on localStorage["highlights.v1"].version — the deterministic readiness signal produced by markSeen(). Suppression test asserts toast count after the storage advances, no wall-clock wait. Reported by CodeRabbit AI on PR #488.
packages/app/e2e/AGENTS.md "Naming Conventions" requires constants in tests to use SCREAMING_SNAKE_CASE. RELEASES_URL_PATTERN and HIGHLIGHTS_KEY already followed; bring the payload fixtures and selector strings in line. Reported by CodeRabbit AI on PR #488.
parseChangelog used to drop releases without an "App Update Notice" / "中文版本" section before sliceHighlights ran, so when an intermediate release lacked an app-facing notice the previous boundary could not be located and the slice spilled into older versions the user had already seen. Example: bumping v1.0.4 → v1.0.5 with v1.0.4 carrying only download links would surface v1.0.3 and v1.0.2 in the toast. Introduce an internal RawRelease carrying the tag for every release plus an optional summary. parseChangelog keeps every tagged release; window slicing runs on the raw tag list, then summary filtering and the five-version cap apply afterwards. Adds a regression test for the exact "previous lacks notice" payload.
Per-release localeUsed allowed a multi-version toast to mix languages: a zh user with newest release carrying a 中文版本 section but an older skipped release carrying only an English notice would see a zh title and action with an English segment in the middle. Spec #486 explicitly forbids title and description ever mixing languages. loadReleaseHighlights now resolves once in the user's locale, then checks whether every selected release stayed in that locale. If any segment fell back, the entire window is re-resolved in English so title, action, and every description segment share a single locale. Adds unit coverage (mixed window collapses to en, all-zh window stays zh) and an e2e regression for the user-facing path.
|
Two P2 findings from the latest review are addressed in this push:
Verification: typecheck 8/8 · unit 22/22 · e2e 7/7 (added 1 for the locale fix). |
Kobalte's swipe-to-dismiss and Escape key paths call close() directly without going through CloseButton's onClick, leaving userDismissed false and skipping onDismiss. On touch devices a swipe-away (or any keyboard Escape) was silently ignored by markSeen, so the release notes toast re-appeared on next launch even after the user had dismissed it. Wire markUserDismissed into onSwipeEnd and onEscapeKeyDown on the Toast root, alongside the existing CloseButton and action-button hooks. Add an e2e test that presses Escape and asserts highlights.v1 advances — the swipe path uses the same hook so this guards both code paths.
|
P1-1 fixed in 5216a8d. Remaining P2/P3 verdicts below — most are cosmetic or self-flagged non-blocking by the reviewer. P1-1 swipe/escape skip markSeen — fixed Verified against
Fix: attach both Validation: typecheck 8/8, unit 937/937, e2e 8/8 (1 new). P2/P3 verdicts
|
Title and the GitHub release link were derived from summaries[0].tag, which diverges from platform.version when the current release lacks a notice in the resolved locale. Concrete failure: zh user updates from v2026.5.5 to v2026.5.7; the v2026.5.7 body has only a 中文版本 section (no App Update Notice), and v2026.5.6 has only an App Update Notice (no 中文版本). First-pass zh resolves to mixed (zh + en), triggers an English rebuild which drops v2026.5.7 (no English notice there), so summaries[0] is v2026.5.6 and the toast shows "Updated to v2026.5.6" linking to the older release. Use platform.version (with v prefix) for both the title and the link URL. summaries[0].localeUsed still drives the title/action copy locale so spec #486's "title and description must always use the same locale" holds. Added e2e covering the failure scenario above.
Two related Toast API fixes: 1) Spec #486 requires that "any dismiss path — close button, action click, or programmatic dismiss — marks the version as seen". Raw toaster.dismiss(id) bypassed userDismissed and skipped onDismiss, so external programmatic dismissals failed the contract. Add a dismissToast(id) wrapper that flags the id via a module-scope set which the render closure's onCleanup consults. Raw toaster.dismiss (used by promise-toast lifecycle, etc.) still counts as ambient teardown, preserving the route/app-exit guard. 2) The action button ran action.onClick() before toaster.dismiss(id). A synchronous throw in onClick would skip the dismiss and pin the toast on screen. Wrap the call in try/finally so dismiss always runs even if the action handler throws.
…ersion When the resolved-locale window drops the current release (e.g. zh user on v2026.5.7 with only a 中文版本 section, fallback to English drops it because v2026.5.7 has no English notice), summaries[0] is an older release. The toast title still reads "Updated to v2026.5.7" — anchored on platform.version — but buildToastDescription assumed summaries[0] was the current version and omitted its tag. Result: the description's first segment looked like the current release's content. Pass currentTag into buildToastDescription and only omit the tag prefix when summaries[i].tag matches it. Strengthen the matching e2e to assert v2026.5.6 appears in the description when it is summaries[0] but not the current version.
|
Review handling: P2 description first segment missing tag (fixed in 3e55c4a)
Fix: pass Verification: typecheck 8/8, unit 937/937, e2e 9/9. P3 docs/DESIGN.md not in PR (declined) `docs/DESIGN.md` is a repo-local file excluded via `.git/info/exclude` (not the tracked .gitignore), so it can never appear in any PR diff. The subtle variant is documented locally in DESIGN.md (zh, line 209). Issue #486's done criterion as worded is unenforceable at the repository level. A PR cannot modify a git-untracked file. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/app/e2e/release-notes/release-notes-toast.spec.ts (1)
220-223: ⚡ Quick winExtract the hardcoded
"pawwork.global.dat:language"key into a named constant.The string appears in four tests verbatim. If the key is ever renamed in the application, these locale tests silently degrade: the
localesetting simply isn't applied, every "falls back to English" assertion vacuously passes, and thezhcontent assertions fail in a confusing way.
settingsKeyalready follows the pattern of being imported/shared; the language key deserves the same treatment — either exported from fixtures or at minimum declared at the top of this file alongside the other constants.♻️ Proposed refactor
const HIGHLIGHTS_KEY = "highlights.v1" +const LANGUAGE_KEY = "pawwork.global.dat:language"Then replace each inline string with
LANGUAGE_KEY, e.g.:- localStorage.setItem("pawwork.global.dat:language", JSON.stringify({ locale: "zh" })) + localStorage.setItem(LANGUAGE_KEY, JSON.stringify({ locale: "zh" }))Also applies to: 244-246, 281-283, 333-336
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/e2e/release-notes/release-notes-toast.spec.ts` around lines 220 - 223, Extract the hardcoded "pawwork.global.dat:language" into a named constant (e.g. LANGUAGE_KEY) and use it wherever localStorage.setItem is called in these tests; either import the existing exported key from the shared fixtures (preferred) or declare const LANGUAGE_KEY = "pawwork.global.dat:language" at the top of the spec file next to other constants like HIGHLIGHTS_KEY/settingsKey, then replace every inline occurrence of "pawwork.global.dat:language" (the four localStorage.setItem usages) with LANGUAGE_KEY so tests will break loudly if the app renames the key.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/app/e2e/release-notes/release-notes-toast.spec.ts`:
- Around line 220-223: Extract the hardcoded "pawwork.global.dat:language" into
a named constant (e.g. LANGUAGE_KEY) and use it wherever localStorage.setItem is
called in these tests; either import the existing exported key from the shared
fixtures (preferred) or declare const LANGUAGE_KEY =
"pawwork.global.dat:language" at the top of the spec file next to other
constants like HIGHLIGHTS_KEY/settingsKey, then replace every inline occurrence
of "pawwork.global.dat:language" (the four localStorage.setItem usages) with
LANGUAGE_KEY so tests will break loudly if the app renames the key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0fdd812b-8573-400b-b107-918270dc2931
📒 Files selected for processing (3)
packages/app/e2e/release-notes/release-notes-toast.spec.tspackages/app/src/context/highlights.tsxpackages/ui/src/components/toast.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/app/src/context/highlights.tsx
- packages/ui/src/components/toast.tsx
Adds an e2e that clicks the Full release notes action and asserts the GitHub release URL is opened (via a stubbed window.open that captures calls) and that the version advances in highlights.v1. Guards three regressions in one test: (1) URL stays anchored on platform.version with the v prefix, (2) action click marks the version as seen via the same onDismiss path used by close/Escape, (3) the toast actually dismisses on action click.
|
P2 (action click coverage) addressed in 9399c5d. Added an e2e that clicks the Full release notes action with a stubbed window.open and asserts (1) the captured URL equals `https://github.com/Astro-Han/pawwork/releases/tag/v2026.5.7\`, (2) highlights.v1 advances via the same onDismiss path used by close/Escape, (3) the toast dismisses. 10/10 e2e local. P3 (docs/DESIGN.md) already addressed in #issuecomment-4396407795: that file is repo-local via .git/info/exclude, never enters PR diff. |
|
Nitpick addressed in ee020cf. Extracted |
1 participant
Summary
Replaces the post-update release notes Dialog with a persistent subtle Toast in the bottom-right. Title shows
Updated to v{version}(no brand prefix); description shows the parser-produced intro and bullets from the GitHub release; one actionFull release notes →opens the GitHub release page; close-from-any-path marks the version seen so it never re-shows.Why
The current 720x400 modal dialog blocks the screen on first launch after every update, forces internal scrolling (which
docs/DESIGN.mdreserves for sheets, not dialogs), and uses a two-column layout for media that releases never ship. Industry trend (Cursor 3, Claude Desktop, Codex in 2026) has moved to lightweight non-blocking notifications linking to a web changelog. The toast is non-blocking, dismissable, and follows DESIGN.md primitives.Related Issue
Closes #486. The locked spec lives in the issue body and the first follow-up comment; the implementation plan lives in the second follow-up comment, edited to reflect each crosscheck round.
Human Review Status
Pending. A human should make the final merge decision after reviewing the final diff and verification evidence.
Review Focus
The new
subtleToast variant inpackages/ui/src/components/toast.{tsx,css}and itsonDismisshook (registered via SolidonCleanupso close button, action click, and programmatic dismiss all converge on one callback). TheTOAST_COPYtable inhighlights.tsxdeliberately duplicates strings from the deleted i18n keys: spec requires title and description to share a locale, and@solid-primitives/i18nhas no per-call locale override; rationale is documented inline. Multi-version description layout (first segment description-only because newest tag is in title; subsequent segments prefix with their tag, separated by a blank line). Decision to drop the never-used structured-highlights parser branch (Array.isArray(value.highlights),MAX_STRUCTURED_HIGHLIGHTS,value.releaseswrapper accessor,parseHighlight/parseMedia/Highlight.media) — verified against the full/releaseshistory before deleting.Risk Notes
User-visible: the dialog is gone on first launch after update; users who relied on the explicit "Don't show these in the future" button now toggle Settings → General → Release notes (the underlying preference and behavior are unchanged). Storage:
highlights.v1schema is unchanged; themarkSeensemantics now run on dismiss instead of show, so a user who quits before dismissing keeps the toast queued for the next launch (matches spec). i18n: fourdialog.releaseNotes.*keys are removed; no other consumers grep up. No platform, packaging, updater, signing, or migration risk.How To Verify
E2E coverage in
packages/app/e2e/release-notes/release-notes-toast.spec.ts: subtle toast appears with correct title/description/icon/action; close button writes markSeen to localStorage;releaseNotes=falsesuppresses; multi-version skipped releases merge with version labels; locale fallback keeps title and action in sync with the parser's effective locale (en title when zh release section is missing); zh title/action when zh release section is present.Note:
bun run dev:desktopcannot exercise the post-update path because the dev build reports "此构建不支持更新" — the Electron updater pipeline that gateshighlightsis disabled in dev. E2E covers the full controller flow with mocked GitHub Releases and seeded localStorage instead, which is the deterministic path.Screenshots or Recordings
Visual verification is captured by the E2E suite (subtle variant render + locale + multi-version). Adding a screenshot of the running toast on top of that would not change the verdict; if a maintainer wants one, I can attach a Playwright trace artifact on request.
Checklist
dev, and my PR title and commit messages use Conventional Commits in EnglishMaintainer label request:
type:feat,area:app,area:ui,priority:p1.Summary by CodeRabbit
New Features
Behavior
Style