[XKCD Comics] Add viewing history#26704
Conversation
raycastbot
added
extension fix / improvement
|
Thank you for your first contribution! 🎉 🔔 @maggie-j-liu you might want to have a look. You can use this guide to learn how to check out the Pull Request locally in order to test it. 📋 Quick checkout commandsBRANCH="xkcd-history"
FORK_URL="https://github.com/ronak-guliani/extensions.git"
EXTENSION_NAME="xkcd"
REPO_NAME="extensions"
git clone -n --depth=1 --filter=tree:0 -b $BRANCH $FORK_URL
cd $REPO_NAME
git sparse-checkout set --no-cone "extensions/$EXTENSION_NAME"
git checkout
cd "extensions/$EXTENSION_NAME"
npm install && npm run devWe're currently experiencing a high volume of incoming requests. As a result, the initial review may take up to 10-15 business days. |
Greptile SummaryThis PR adds a viewing history feature to the XKCD Comics extension, persisting up to 100 entries in Key issues found:
Confidence Score: 3/5Not safe to merge — the history view's own navigation corrupts the history data on every use. There is a P1 logic bug where selecting any item in
Important Files Changed
Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/xkcd/src/history.tsx
Line: 56-68
Comment:
**Browsing history view corrupts history data**
When a user selects an item in `HistoryView`, `setCurrentComic(num)` updates the shared `currentComicAtom`. Because `index.tsx` also subscribes to this atom via `useCurrentSelectedComic(currentComicNumber)`, it fetches that comic and its `useEffect([currentComic])` fires — adding the entry to history with a fresh timestamp and moving it to the top of the list.
This means simply scrolling through the history view (every selection change triggers `onSelectionChange`) re-timestamps and re-orders every hovered entry, completely corrupting the historical viewing order.
The fix is to decouple the history view's comic-fetching from the shared `currentComicAtom`. For example, use a local `useState` for the selected comic number in `HistoryView`:
```tsx
// In HistoryView — use local state instead of the shared atom
const [selectedNum, setSelectedNum] = useState<number>(-1);
const [currentComic, loadingComic] = useCurrentSelectedComic(selectedNum);
const onSelectionChange = useCallback((id: string | undefined) => {
if (!id || selectedId.current === id) return;
selectedId.current = id;
const num = Number(id);
if (!isNaN(num)) setSelectedNum(num); // local state, not shared atom
}, []);
```
This way the history view can display a comic's detail without triggering the read-tracking side effects in `index.tsx`.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/xkcd/src/index.tsx
Line: 30-33
Comment:
**Stale `history` closure in useEffect**
`history` is read inside this effect but is not listed in the dependency array (`[currentComic]`). If `setHistory(parsed)` from the LocalStorage loading effect fires concurrently with this effect, the `history.filter(...)` call may operate on the stale empty initial state `[]`, silently dropping all previously-persisted history entries before they are written back to storage.
Consider using a functional updater to always read the latest atom state:
```tsx
setHistory((prev) => {
const newEntry = { num: currentComic.num, viewedAt: new Date().toISOString() };
const updated = [newEntry, ...prev.filter((e) => e.num !== currentComic.num)].slice(0, 100);
LocalStorage.setItem("history", JSON.stringify(updated));
return updated;
});
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/xkcd/src/index.tsx
Line: 119
Comment:
**`history` prop snapshot may go stale in `HistoryView`**
`<HistoryView history={history} />` passes a snapshot of the atom value at the time the parent last rendered. If `historyAtom` is updated while the user is inside `HistoryView`, the displayed list won't reflect those changes because `HistoryView` doesn't subscribe to the atom.
A simpler, more resilient pattern is to read `historyAtom` directly inside `HistoryView` with `useAtom`, removing the `history` prop entirely:
```tsx
// In history.tsx
const [history] = useAtom(historyAtom);
```
This applies to all five `<HistoryView history={history} />` call sites in `index.tsx`.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/xkcd/src/history.tsx
Line: 60
Comment:
**`accessoryTitle` is deprecated**
`accessoryTitle` was deprecated in the Raycast API in favor of the `accessories` array prop. Since this is a new file it's a good opportunity to use the current API:
```suggestion
accessories={[{ text: relativeTime(entry.viewedAt) }]}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "xkcd: add history tracking and View Hist..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| <List.Item | ||
| id={entry.num.toString()} | ||
| key={entry.num} | ||
| title={`Comic #${entry.num}`} | ||
| accessoryTitle={relativeTime(entry.viewedAt)} | ||
| detail={detail} | ||
| actions={ | ||
| <ActionPanel> | ||
| <OpenComicInBrowser /> | ||
| <ExplainXkcd /> | ||
| </ActionPanel> | ||
| } | ||
| /> |
There was a problem hiding this comment.
When a user selects an item in HistoryView, setCurrentComic(num) updates the shared currentComicAtom. Because index.tsx also subscribes to this atom via useCurrentSelectedComic(currentComicNumber), it fetches that comic and its useEffect([currentComic]) fires — adding the entry to history with a fresh timestamp and moving it to the top of the list.
This means simply scrolling through the history view (every selection change triggers onSelectionChange) re-timestamps and re-orders every hovered entry, completely corrupting the historical viewing order.
The fix is to decouple the history view's comic-fetching from the shared currentComicAtom. For example, use a local useState for the selected comic number in HistoryView:
// In HistoryView — use local state instead of the shared atom
const [selectedNum, setSelectedNum] = useState<number>(-1);
const [currentComic, loadingComic] = useCurrentSelectedComic(selectedNum);
const onSelectionChange = useCallback((id: string | undefined) => {
if (!id || selectedId.current === id) return;
selectedId.current = id;
const num = Number(id);
if (!isNaN(num)) setSelectedNum(num); // local state, not shared atom
}, []);This way the history view can display a comic's detail without triggering the read-tracking side effects in index.tsx.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/xkcd/src/history.tsx
Line: 56-68
Comment:
**Browsing history view corrupts history data**
When a user selects an item in `HistoryView`, `setCurrentComic(num)` updates the shared `currentComicAtom`. Because `index.tsx` also subscribes to this atom via `useCurrentSelectedComic(currentComicNumber)`, it fetches that comic and its `useEffect([currentComic])` fires — adding the entry to history with a fresh timestamp and moving it to the top of the list.
This means simply scrolling through the history view (every selection change triggers `onSelectionChange`) re-timestamps and re-orders every hovered entry, completely corrupting the historical viewing order.
The fix is to decouple the history view's comic-fetching from the shared `currentComicAtom`. For example, use a local `useState` for the selected comic number in `HistoryView`:
```tsx
// In HistoryView — use local state instead of the shared atom
const [selectedNum, setSelectedNum] = useState<number>(-1);
const [currentComic, loadingComic] = useCurrentSelectedComic(selectedNum);
const onSelectionChange = useCallback((id: string | undefined) => {
if (!id || selectedId.current === id) return;
selectedId.current = id;
const num = Number(id);
if (!isNaN(num)) setSelectedNum(num); // local state, not shared atom
}, []);
```
This way the history view can display a comic's detail without triggering the read-tracking side effects in `index.tsx`.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed — HistoryView now uses local useState for the selected comic number instead of the shared currentComicAtom. Browsing the history list no longer triggers the read-tracking useEffect in index.tsx.
extensions/xkcd/src/index.tsx
Outdated
| const newEntry = { num: currentComic.num, viewedAt: new Date().toISOString() }; | ||
| const updated = [newEntry, ...history.filter((e) => e.num !== currentComic.num)].slice(0, 100); | ||
| setHistory(updated); | ||
| LocalStorage.setItem("history", JSON.stringify(updated)); |
There was a problem hiding this comment.
history closure in useEffect
history is read inside this effect but is not listed in the dependency array ([currentComic]). If setHistory(parsed) from the LocalStorage loading effect fires concurrently with this effect, the history.filter(...) call may operate on the stale empty initial state [], silently dropping all previously-persisted history entries before they are written back to storage.
Consider using a functional updater to always read the latest atom state:
setHistory((prev) => {
const newEntry = { num: currentComic.num, viewedAt: new Date().toISOString() };
const updated = [newEntry, ...prev.filter((e) => e.num !== currentComic.num)].slice(0, 100);
LocalStorage.setItem("history", JSON.stringify(updated));
return updated;
});Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/xkcd/src/index.tsx
Line: 30-33
Comment:
**Stale `history` closure in useEffect**
`history` is read inside this effect but is not listed in the dependency array (`[currentComic]`). If `setHistory(parsed)` from the LocalStorage loading effect fires concurrently with this effect, the `history.filter(...)` call may operate on the stale empty initial state `[]`, silently dropping all previously-persisted history entries before they are written back to storage.
Consider using a functional updater to always read the latest atom state:
```tsx
setHistory((prev) => {
const newEntry = { num: currentComic.num, viewedAt: new Date().toISOString() };
const updated = [newEntry, ...prev.filter((e) => e.num !== currentComic.num)].slice(0, 100);
LocalStorage.setItem("history", JSON.stringify(updated));
return updated;
});
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed — switched to a functional updater setHistory(prev => [...]) so the effect always operates on the latest state regardless of when the closure was captured.
extensions/xkcd/src/index.tsx
Outdated
| <ActionPanel> | ||
| <OpenComicInBrowser /> | ||
| <ExplainXkcd /> | ||
| <Action.Push title="View History" icon={Icon.Clock} target={<HistoryView history={history} />} /> |
There was a problem hiding this comment.
history prop snapshot may go stale in HistoryView
<HistoryView history={history} /> passes a snapshot of the atom value at the time the parent last rendered. If historyAtom is updated while the user is inside HistoryView, the displayed list won't reflect those changes because HistoryView doesn't subscribe to the atom.
A simpler, more resilient pattern is to read historyAtom directly inside HistoryView with useAtom, removing the history prop entirely:
// In history.tsx
const [history] = useAtom(historyAtom);This applies to all five <HistoryView history={history} /> call sites in index.tsx.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/xkcd/src/index.tsx
Line: 119
Comment:
**`history` prop snapshot may go stale in `HistoryView`**
`<HistoryView history={history} />` passes a snapshot of the atom value at the time the parent last rendered. If `historyAtom` is updated while the user is inside `HistoryView`, the displayed list won't reflect those changes because `HistoryView` doesn't subscribe to the atom.
A simpler, more resilient pattern is to read `historyAtom` directly inside `HistoryView` with `useAtom`, removing the `history` prop entirely:
```tsx
// In history.tsx
const [history] = useAtom(historyAtom);
```
This applies to all five `<HistoryView history={history} />` call sites in `index.tsx`.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed — HistoryView now reads historyAtom directly via useAtom instead of receiving a snapshot prop. The prop has been removed from all call sites.
extensions/xkcd/src/history.tsx
Outdated
| id={entry.num.toString()} | ||
| key={entry.num} | ||
| title={`Comic #${entry.num}`} | ||
| accessoryTitle={relativeTime(entry.viewedAt)} |
There was a problem hiding this comment.
accessoryTitle is deprecated
accessoryTitle was deprecated in the Raycast API in favor of the accessories array prop. Since this is a new file it's a good opportunity to use the current API:
| accessoryTitle={relativeTime(entry.viewedAt)} | |
| accessories={[{ text: relativeTime(entry.viewedAt) }]} |
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/xkcd/src/history.tsx
Line: 60
Comment:
**`accessoryTitle` is deprecated**
`accessoryTitle` was deprecated in the Raycast API in favor of the `accessories` array prop. Since this is a new file it's a good opportunity to use the current API:
```suggestion
accessories={[{ text: relativeTime(entry.viewedAt) }]}
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed — replaced accessoryTitle with the current accessories={[{ text: relativeTime(entry.viewedAt) }]} API.
…tHistory updater, read historyAtom directly, replace deprecated accessoryTitle
…tHistory updater, read historyAtom directly, replace deprecated accessoryTitle
|
All four review comments addressed in the latest push:
|
2 participants
Description
Adds a viewing history feature to the XKCD Comics extension.
Checklist