feat(sidebar): Attention widget for major release/breaking change blog posts#6631
Open
siduck wants to merge 2 commits into
Open
feat(sidebar): Attention widget for major release/breaking change blog posts#6631siduck wants to merge 2 commits into
siduck wants to merge 2 commits into
Conversation
Contributor
|
| Filename | Overview |
|---|---|
| dashboard/src/components/navigation/sidebar/AttentionWidget.vue | New widget that fetches /releases, parses HTML via regex for data-major links, and shows a banner; has a null-href bug that breaks the widget when no major post is found, missing fetch error handling, and a CSS typo. |
| dashboard/src/components/navigation/sidebar/Sidebar.vue | Adds AttentionWidget import/usage and removes mt-auto from the bottom bar (now handled by a spacer div inside AttentionWidget); change is minimal and safe. |
Sequence Diagram
sequenceDiagram
participant Browser
participant AttentionWidget
participant ReleasesPage as /releases (HTML)
participant LocalStorage
Browser->>AttentionWidget: onMounted
AttentionWidget->>ReleasesPage: fetch('/releases')
ReleasesPage-->>AttentionWidget: HTML response
AttentionWidget->>AttentionWidget: "regex match data-major=true href"
AttentionWidget->>LocalStorage: read latestReleasePath
alt "href !== localStorage.latestReleasePath"
AttentionWidget->>Browser: show banner
alt user clicks New Releases
Browser->>AttentionWidget: openUrl()
AttentionWidget->>Browser: window.open(https://cloud.frappe.io + href)
AttentionWidget->>LocalStorage: "write latestReleasePath = href"
else user clicks dismiss
Browser->>AttentionWidget: setLastBlogPathname()
AttentionWidget->>LocalStorage: "write latestReleasePath = href"
end
end
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
dashboard/src/components/navigation/sidebar/AttentionWidget.vue:16-19
When no `data-major="true"` link exists on the releases page, `href` is `null`. Since `localStorage.latestReleasePath` is `undefined` for first-time visitors, `undefined !== null` is `true`, so `show` becomes `true` and `latestPathName` stays `null`. Clicking "New Releases" then opens `https://cloud.frappe.ionull`. Worse, `setLastBlogPathname` stores the string `"null"` via `localStorage.latestReleasePath = null`, so the widget re-shows on every subsequent visit (since `"null" !== null`).
```suggestion
if (href && localStorage?.latestReleasePath !== href) {
latestPathName.value = href
show.value = true
}
```
### Issue 2 of 3
dashboard/src/components/navigation/sidebar/AttentionWidget.vue:8-11
The fetch to `/releases` has no error handling — a network failure or non-200 response will produce an unhandled promise rejection that silently breaks the component lifecycle.
```suggestion
const getLatestMajorBlogPath = async () => {
const url = '/releases'
try {
const res = await fetch(url)
if (!res.ok) return
const html = await res.text()
```
### Issue 3 of 3
dashboard/src/components/navigation/sidebar/AttentionWidget.vue:40
`hadow` is a typo — Tailwind will not recognize it, so the intended shadow styling is silently dropped.
```suggestion
class="hidden md:flex flex-col gap-2 p-3 border border-outline-gray-2 dark:border-outline-gray-1 rounded text-sm bg-surface-white dark:bg-surface-cards shadow mt-auto mb-1"
```
Reviews (1): Last reviewed commit: "fix(attention-widget): add .value inside..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+16
to
+19
| if (localStorage?.latestReleasePath !== href) { | ||
| latestPathName.value = href | ||
| show.value = true | ||
| } |
Contributor
There was a problem hiding this comment.
data-major="true" link exists on the releases page, href is null. Since localStorage.latestReleasePath is undefined for first-time visitors, undefined !== null is true, so show becomes true and latestPathName stays null. Clicking "New Releases" then opens https://cloud.frappe.ionull. Worse, setLastBlogPathname stores the string "null" via localStorage.latestReleasePath = null, so the widget re-shows on every subsequent visit (since "null" !== null).
Suggested change
| if (localStorage?.latestReleasePath !== href) { | |
| latestPathName.value = href | |
| show.value = true | |
| } | |
| if (href && localStorage?.latestReleasePath !== href) { | |
| latestPathName.value = href | |
| show.value = true | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: dashboard/src/components/navigation/sidebar/AttentionWidget.vue
Line: 16-19
Comment:
When no `data-major="true"` link exists on the releases page, `href` is `null`. Since `localStorage.latestReleasePath` is `undefined` for first-time visitors, `undefined !== null` is `true`, so `show` becomes `true` and `latestPathName` stays `null`. Clicking "New Releases" then opens `https://cloud.frappe.ionull`. Worse, `setLastBlogPathname` stores the string `"null"` via `localStorage.latestReleasePath = null`, so the widget re-shows on every subsequent visit (since `"null" !== null`).
```suggestion
if (href && localStorage?.latestReleasePath !== href) {
latestPathName.value = href
show.value = true
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+8
to
+11
| const getLatestMajorBlogPath = async () => { | ||
| const url = '/releases' | ||
| const res = await fetch(url) | ||
| const html = await res.text() |
Contributor
There was a problem hiding this comment.
/releases has no error handling — a network failure or non-200 response will produce an unhandled promise rejection that silently breaks the component lifecycle.
Suggested change
| const getLatestMajorBlogPath = async () => { | |
| const url = '/releases' | |
| const res = await fetch(url) | |
| const html = await res.text() | |
| const getLatestMajorBlogPath = async () => { | |
| const url = '/releases' | |
| try { | |
| const res = await fetch(url) | |
| if (!res.ok) return | |
| const html = await res.text() |
Prompt To Fix With AI
This is a comment left during a code review.
Path: dashboard/src/components/navigation/sidebar/AttentionWidget.vue
Line: 8-11
Comment:
The fetch to `/releases` has no error handling — a network failure or non-200 response will produce an unhandled promise rejection that silently breaks the component lifecycle.
```suggestion
const getLatestMajorBlogPath = async () => {
const url = '/releases'
try {
const res = await fetch(url)
if (!res.ok) return
const html = await res.text()
```
How can I resolve this? If you propose a fix, please make it concise.| <template> | ||
| <div class="mt-auto" /> | ||
| <div | ||
| class="hidden md:flex flex-col gap-2 p-3 border border-outline-gray-2 dark:border-outline-gray-1 rounded text-sm bg-surface-white dark:bg-surface-cards hadow mt-auto mb-1" |
Contributor
There was a problem hiding this comment.
hadow is a typo — Tailwind will not recognize it, so the intended shadow styling is silently dropped.
Suggested change
| class="hidden md:flex flex-col gap-2 p-3 border border-outline-gray-2 dark:border-outline-gray-1 rounded text-sm bg-surface-white dark:bg-surface-cards hadow mt-auto mb-1" | |
| class="hidden md:flex flex-col gap-2 p-3 border border-outline-gray-2 dark:border-outline-gray-1 rounded text-sm bg-surface-white dark:bg-surface-cards shadow mt-auto mb-1" |
Prompt To Fix With AI
This is a comment left during a code review.
Path: dashboard/src/components/navigation/sidebar/AttentionWidget.vue
Line: 40
Comment:
`hadow` is a typo — Tailwind will not recognize it, so the intended shadow styling is silently dropped.
```suggestion
class="hidden md:flex flex-col gap-2 p-3 border border-outline-gray-2 dark:border-outline-gray-1 rounded text-sm bg-surface-white dark:bg-surface-cards shadow mt-auto mb-1"
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6631 +/- ##
============================================
+ Coverage 50.18% 62.88% +12.70%
============================================
Files 990 118 -872
Lines 82967 18081 -64886
Branches 523 533 +10
============================================
- Hits 41633 11370 -30263
+ Misses 41302 6678 -34624
- Partials 32 33 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
Screenshot
Description
But! Not every blog post on https://cloud.frappe.io/releases will have major changes or is worthy of user's attention. So how do we determine the
latest major blog?data-major="true"And how will the link get
data-majorattribute? from the metadata of the page itself !!The static site is written in Astro! https://github.com/frappe/press/tree/master/changelog
Note: right now the user's last visited releasepage* url is stored in localstorage and the widget is hidden on mobiles, I can add a custom field in User doctype but not sure if it'll get messy!