fix: open existing threads at the bottom instantly, no smooth scroll animation#36
Conversation
… on PATH Two fixes: 1. Scroll persistence: when switching threads, read the saved per-session pinned state from lastTimelinePinnedBySessionRef instead of unconditionally resetting pinnedToBottomRef to true. The save/restore infrastructure already exists; the unconditional reset was overriding it. 2. npm package loading: when pi-gui is launched via Finder/Dock on macOS, the Electron process inherits a minimal PATH that does not include /usr/local/bin or /opt/homebrew/bin. This causes "npm root -g" to fail, which triggers a silent fallback that strips all npm: Pi package sources, preventing extensions like pi-cymbal from loading. Prepend common macOS npm installation paths to PATH early in the Electron main process. Fixes minghinmatthewlam#34, fixes minghinmatthewlam#35
|
@pechhe is attempting to deploy a commit to the dev Team on Vercel. A member of the Team first needs to authorize it. |
![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: 4284708ade
ℹ️ 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".
| pinnedToBottomRef.current = | ||
| lastTimelinePinnedBySessionRef.current.get(selectedSessionKey) ?? true; |
There was a problem hiding this comment.
When switching threads, the store emits the new selected state before it publishes the selected transcript (app-store.ts publishes the transcript after emit()), so this effect can set pinnedToBottomRef to false while activeTranscript is still the empty loading view. The ref restore then applies savedScrollTop to that empty pane, and once the real transcript arrives this new false value prevents the later bottom/restore alignment from running, leaving revisited scrolled-up threads at scrollTop === 0 rather than their saved offset. This shows up specifically when the selected transcript update is asynchronous from the state update; delay applying the saved unpinned state or rerun the saved-scroll restore once the matching transcript is loaded.
Useful? React with 👍 / 👎.
…animation The .timeline-pane CSS declared `scroll-behavior: smooth`, which turned every programmatic `pane.scrollTop = X` assignment into a smooth-scroll animation. When the user opened an existing thread the timeline would animate top-to-bottom instead of landing at the chat end immediately. Change `scroll-behavior` to `auto` on the timeline pane. Callers that need a smooth animation (the "Jump to latest" button) already pass an explicit `behavior: "smooth"` to `pane.scrollTo()` so they are unaffected. Add a desktop-core regression test that samples the scroll position over the first ~12 frames after opening a thread and asserts no frame departs from the bottom by more than 60 px.
pechhe
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e155293d4
ℹ️ 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 currentPath = process.env.PATH ?? ""; | ||
| const missingPaths = extraBinPaths.filter((p) => !currentPath.split(":").includes(p)); | ||
| if (missingPaths.length > 0) { | ||
| process.env.PATH = [...missingPaths, currentPath].join(":"); |
There was a problem hiding this comment.
When the app is launched from a terminal with a working user-managed npm already first in PATH (for example nvm/asdf), this assignment still prepends Homebrew/global paths whenever they are missing. That changes which npm backs npm root -g, so npm package sources installed under the user's original npm can stop resolving in pi-gui even though terminal pi works. Only add these fallback paths after the existing PATH, or only prepend them for the known minimal Finder/Dock environment.
Useful? React with 👍 / 👎.
1 participant
The
.timeline-paneCSS declaredscroll-behavior: smooth, which turned every programmaticpane.scrollTop = Xassignment into a smooth-scroll animation. When the user opened an existing thread, the timeline would animate top-to-bottom instead of landing at the chat end immediately.Root cause
CSS
scroll-behavior: smoothon.timeline-panecaused the browser to animate every programmatic scroll — includingpane.scrollTop = pane.scrollHeightduring transcript restoration — into a smooth transition from the current position. On a thread switch the pane starts atscrollTop=0(top of the loaded transcript), and the bottom-alignment would trigger a visible smooth scroll from top to bottom.What changed
scroll-behavior: smooth→autoon.timeline-pane. Callers that need smooth scroll (the "Jump to latest" button) already passbehavior: "smooth"explicitly topane.scrollTo(), so they are unaffected.timeline-pinning.spec.tsthat samples the scroll position over ~12 frames after opening a thread and asserts no frame departs from the bottom by more than 60 px.How to verify