Fix sidebar task scroll layout#2362
Open
Basit-Balogun10 wants to merge 2 commits into
Open
Conversation
Contributor
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+96
to
+246
| // Diagnostic: inspect the structure of entries to debug filterEntriesUpToMessage | ||
| { | ||
| let sessionUpdateCount = 0; | ||
| let userChunkCount = 0; | ||
| let userMessageGroups = 0; | ||
| let inUserMsg = false; | ||
| const sampleMethods: string[] = []; | ||
| const sampleSessionUpdates: string[] = []; | ||
|
|
||
| for (const e of allEntries) { | ||
| const method = e.notification?.method; | ||
| if (sampleMethods.length < 10 && method && !sampleMethods.includes(method)) { | ||
| sampleMethods.push(method); | ||
| } | ||
| const p = e.notification?.params as Record<string, unknown> | undefined; | ||
| if (method === "session/update" && p?.update) { | ||
| sessionUpdateCount++; | ||
| const u = p.update as { sessionUpdate?: string }; | ||
| if (u.sessionUpdate && sampleSessionUpdates.length < 15 && !sampleSessionUpdates.includes(u.sessionUpdate)) { | ||
| sampleSessionUpdates.push(u.sessionUpdate); | ||
| } | ||
| const isUC = u.sessionUpdate === "user_message" || u.sessionUpdate === "user_message_chunk"; | ||
| if (isUC) { | ||
| userChunkCount++; | ||
| if (!inUserMsg) { userMessageGroups++; inUserMsg = true; } | ||
| } else { | ||
| inUserMsg = false; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| log.info("Entry structure diagnostic", { | ||
| totalEntries: allEntries.length, | ||
| sessionUpdateEntries: sessionUpdateCount, | ||
| userChunkEntries: userChunkCount, | ||
| userMessageGroups, | ||
| targetMessageIndex: forkAtMessageIndex, | ||
| sampleMethods, | ||
| sampleSessionUpdates, | ||
| firstEntryKeys: allEntries[0] ? Object.keys(allEntries[0]) : [], | ||
| firstEntryNotificationKeys: allEntries[0]?.notification ? Object.keys(allEntries[0].notification) : [], | ||
| firstEntryType: allEntries[0]?.type, | ||
| }); | ||
| } | ||
|
|
||
| log.info("Fetched source entries", { | ||
| sourceTaskRunId, | ||
| entryCount: allEntries.length, | ||
| }); | ||
|
|
||
| // 2. Slice entries up to and including the response to message N | ||
| const entries = filterEntriesUpToMessage(allEntries, forkAtMessageIndex); | ||
|
|
||
| // Diagnostic: show what happened around the cutoff | ||
| { | ||
| const cutoffIdx = entries.length; | ||
| const didTruncate = cutoffIdx < allEntries.length; | ||
|
|
||
| // Describe the last 5 included entries | ||
| const lastIncluded = entries.slice(-5).map((e, i) => { | ||
| const idx = cutoffIdx - 5 + i; | ||
| const method = e.notification?.method ?? "?"; | ||
| const p = e.notification?.params as Record<string, unknown> | undefined; | ||
| const u = (p?.update as { sessionUpdate?: string })?.sessionUpdate; | ||
| return `[${idx}] ${method}${u ? `:${u}` : ""}`; | ||
| }); | ||
|
|
||
| // Describe the first 5 excluded entries | ||
| const firstExcluded = allEntries.slice(cutoffIdx, cutoffIdx + 5).map((e, i) => { | ||
| const idx = cutoffIdx + i; | ||
| const method = e.notification?.method ?? "?"; | ||
| const p = e.notification?.params as Record<string, unknown> | undefined; | ||
| const u = (p?.update as { sessionUpdate?: string })?.sessionUpdate; | ||
| return `[${idx}] ${method}${u ? `:${u}` : ""}`; | ||
| }); | ||
|
|
||
| // Find all user message group boundaries for context | ||
| const userMsgGroupPositions: { groupIndex: number; startIdx: number; chunkCount: number }[] = []; | ||
| let inUM = false; | ||
| let groupIdx = 0; | ||
| let groupStart = 0; | ||
| let groupChunks = 0; | ||
| for (let ei = 0; ei < allEntries.length; ei++) { | ||
| const e = allEntries[ei]; | ||
| const p = e.notification?.params as Record<string, unknown> | undefined; | ||
| if (e.notification?.method === "session/update" && p?.update) { | ||
| const u = (p.update as { sessionUpdate?: string }).sessionUpdate; | ||
| const isChunk = u === "user_message" || u === "user_message_chunk"; | ||
| if (isChunk) { | ||
| if (!inUM) { | ||
| groupStart = ei; | ||
| groupChunks = 1; | ||
| inUM = true; | ||
| } else { | ||
| groupChunks++; | ||
| } | ||
| } else { | ||
| if (inUM) { | ||
| userMsgGroupPositions.push({ groupIndex: groupIdx, startIdx: groupStart, chunkCount: groupChunks }); | ||
| groupIdx++; | ||
| inUM = false; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| if (inUM) { | ||
| userMsgGroupPositions.push({ groupIndex: groupIdx, startIdx: groupStart, chunkCount: groupChunks }); | ||
| } | ||
|
|
||
| // Show groups around the target | ||
| const targetGroup = userMsgGroupPositions[forkAtMessageIndex]; | ||
| const nearbyGroups = userMsgGroupPositions.slice( | ||
| Math.max(0, forkAtMessageIndex - 1), | ||
| Math.min(userMsgGroupPositions.length, forkAtMessageIndex + 3), | ||
| ); | ||
|
|
||
| log.info("filterEntriesUpToMessage result", { | ||
| totalEntries: allEntries.length, | ||
| includedEntries: entries.length, | ||
| excludedEntries: allEntries.length - entries.length, | ||
| didTruncate, | ||
| cutoffIndex: cutoffIdx, | ||
| targetMessageIndex: forkAtMessageIndex, | ||
| targetGroup: targetGroup ?? "NOT FOUND", | ||
| nearbyGroups, | ||
| lastIncluded, | ||
| firstExcluded: didTruncate ? firstExcluded : "(none — all included)", | ||
| }); | ||
| } | ||
|
|
||
| // 3. Find git HEAD SHA from the last checkpoint entry in the filtered set | ||
| const headSha = | ||
| this.extractHeadSha(entries) ?? | ||
| (await this.getCurrentHead(sourceWorktreePath)); | ||
|
|
||
| if (!headSha) { | ||
| throw new Error( | ||
| `Cannot determine git HEAD for fork at message ${forkAtMessageIndex} (sourceTaskRunId: ${sourceTaskRunId})`, | ||
| ); | ||
| } | ||
|
|
||
| // 4. Create the new worktree + workspace record | ||
| const workspace = await this.workspaceService.createWorkspaceFromFork({ | ||
| taskId: newTaskId, | ||
| mainRepoPath, | ||
| headSha, | ||
| }); | ||
|
|
||
| // 5. Record the lineage so the UI can show "Forked from: [title]" | ||
| // Non-fatal: if the migration hasn't run yet the banner simply won't show. | ||
| try { |
Contributor
There was a problem hiding this comment.
Two large blocks explicitly labelled "Diagnostic: …" (lines 96–139 and 149–246) were left in prepareFork. They run on every fork invocation, iterate over the full allEntries array twice, build detailed in-memory data structures (userMsgGroupPositions, etc.), and call log.info with sampleMethods and sampleSessionUpdates — fields that can contain raw conversation content. In production these add measurable CPU overhead per fork and expose conversation data in logs.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/main/services/fork/service.ts
Line: 96-246
Comment:
**Debug diagnostic blocks shipped to production**
Two large blocks explicitly labelled `"Diagnostic: …"` (lines 96–139 and 149–246) were left in `prepareFork`. They run on every fork invocation, iterate over the full `allEntries` array twice, build detailed in-memory data structures (`userMsgGroupPositions`, etc.), and call `log.info` with `sampleMethods` and `sampleSessionUpdates` — fields that can contain raw conversation content. In production these add measurable CPU overhead per fork and expose conversation data in logs.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
89
to
101
| loading: (title: ReactNode, description?: string) => { | ||
| return sonnerToast.custom((id) => ( | ||
| <ToastComponent | ||
| id={id} | ||
| type="loading" | ||
| title={title} | ||
| description={description} | ||
| /> | ||
| )); | ||
| return sonnerToast.custom( | ||
| (id) => ( | ||
| <ToastComponent | ||
| id={id} | ||
| type="loading" | ||
| title={title} | ||
| description={description} | ||
| /> | ||
| ), | ||
| { duration: Infinity }, | ||
| ); | ||
| }, |
Contributor
There was a problem hiding this comment.
toast.loading changed to duration: Infinity — existing callers that never dismiss will produce immortal toasts
toast.loading now never auto-dismisses. Since the component already hides the close button for type="loading", there is no way for a user to clear a stuck toast. Any existing call site that captures the returned ID but fails to later call toast.success/toast.error with { id } (e.g. because an exception escapes the try block, or the function returns early through an unguarded path) will leave a permanently-visible spinner in the UI. The fork flow is correctly wired, but this global change silently raises the stakes for every other caller.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/renderer/utils/toast.tsx
Line: 89-101
Comment:
**Global `toast.loading` changed to `duration: Infinity` — existing callers that never dismiss will produce immortal toasts**
`toast.loading` now never auto-dismisses. Since the component already hides the close button for `type="loading"`, there is no way for a user to clear a stuck toast. Any existing call site that captures the returned ID but fails to later call `toast.success`/`toast.error` with `{ id }` (e.g. because an exception escapes the try block, or the function returns early through an unguarded path) will leave a permanently-visible spinner in the UI. The fork flow is correctly wired, but this global change silently raises the stakes for every other caller.
How can I resolve this? If you propose a fix, please make it concise.
- Top nav icons (new task, search, inbox, skills, etc.) now sit in a shrink-0 flex child so they never scroll away - Task list moved into min-h-0 flex-1 ScrollArea so it fills the remaining height and scrolls independently - Tasks section header (search + filter icons) is sticky top-0 within the scroll area
Basit-Balogun10
force-pushed
the
fix-taskbar-task-scroll
branch
from
85a6cf6 to
a3b09dc
Compare
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.
Problem
The Tasks section in the sidebar was not height-constrained, so scrolling let it expand over the fixed controls instead of staying inside its intended area.
Closes #2359
Changes
Split the sidebar into a fixed controls block and a bounded task scroll region with a sticky task section header (section title "TASKS", search and filter icons)
How did you test this?
Verified the edited files and manually reviewed the layout change.
Demo
sidebar-tasks-section-overflow-scroll-fixed-with-sticky-controls-block-and-tasks-section-header.webm
Publish to changelog?
Do not publish to changelog.