fix(app): stabilize session timeline scroll#286
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe changes add an E2E test file validating session timeline scrolling and rendering behavior across navigation and session switching. Two adjustments support this: the initial message window size increased from 1 to 10, and hash-clearing logic refined to track the specific hash value being cleared to prevent unwanted scroll re-triggering. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces new E2E tests for session scroll positions and increases the initial message rendering count in the timeline from 1 to 10. It also modifies the hash scrolling logic to prevent re-application during clearing. A potential issue was identified in the scroll logic where the 'clearing' flag might persist indefinitely if the URL hash transitions directly between different values without becoming empty, which could block subsequent hash-based scrolling.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Astro-Han
left a comment
There was a problem hiding this comment.
Review
P0 — Potential correctness issue
use-session-hash-scroll.ts — clearing flag not reset on hash change
The new else if (clearing) { return } guard prevents re-applying a stale hash while clearing === true, but clearing is only ever set to false when !hash. If the user navigates to a different hash (e.g. #message-A → #message-B) while clearing is still true, the effect bails out and the new hash is never applied. The flag should probably be keyed to the specific hash being cleared, or reset whenever hash changes, not just when it becomes empty.
P1 — Maintainability / test hygiene
session-scroll-position.spec.ts — seedSessionTurns uses noReply: true then asserts on assistant message count
seedSessionTurns seeds 14 user turns with noReply: true, so there are no assistant messages. The test then asserts toHaveCount(10) on sessionMessageItemSelector (which matches [data-message-id]). This works only because the selector matches user messages too, but the count "10" is the capped window size, not a message-type count. A comment or a more explicit assertion (e.g. counting only .user or .assistant roles) would make the intent obvious and prevent future breakage if the selector scope changes.
session-scroll-position.spec.ts — secondIDs filters on role === "user" but rendered includes all messages
The test asserts rendered.every((id) => secondIDs.has(id)). Since secondIDs is built from user messages only, this will fail if any assistant message is rendered in the window. The test happens to pass because the 10-turn window ends on a user message boundary, but this coupling is fragile. Assert against all message IDs, not just user ones.
session-send-stability.spec.ts — MAX_STABILITY_SAMPLES = 256 is a magic number with no explanation
Why 256? If a test generates more than 256 mutations/samplings, the probe silently drops data. A brief comment on how this bound was chosen (e.g. "empirically ~120 samples for a 14-turn session switch") would help future maintainers know whether to bump it.
P2 — Code clarity / defensive programming
use-session-hash-scroll.ts — clearing is a module-level mutable flag in a hook factory
let clearing = false lives outside the reactive system. Solid effects can re-run out of order or be batched; a plain let flag is prone to stale-state bugs if multiple rapid hash changes interleave. Consider whether clearing should be a signal or at least scoped to a ref object so its lifetime is explicit.
session-scroll-position.spec.ts — timelineMetrics throws if viewport not found
throw new Error("session timeline viewport not found") inside page.evaluate produces a generic Playwright error with no stack context. Prefer returning a sentinel (e.g. null) and asserting on it in the test body so the failure message points to the right line.
session-scroll-position.spec.ts — installMessageCountProbe leaks maxSamples into page scope
The probe object is injected with { maxSamples: 256, messageSelector: ... }. maxSamples is not used inside the evaluated function — the closure already captures maxSamples from the outer scope. The parameter is dead weight and confusing; remove it or use it.
session-send-stability.spec.ts — recordRemovedMount mutates unmountCount even when bounded
pushBounded silently drops samples once maxSamples is reached, but unmountCount keeps incrementing. This means unmountCount can exceed unmounts.length, which is surprising. Either cap unmountCount alongside the sample list or document the intentional divergence.
P3 — Nitpicks
message-timeline.tsx — stageCfg change from { init: 1, batch: 3 } to { init: 10, batch: 3 } is unexplained in code
The JSDoc on createTimelineStaging says "Defer-mounts small timeline windows so revealing older turns does not block first paint". Bumping init from 1 to 10 means the first paint now mounts up to 10 turns immediately. This trades first-paint speed for no-flicker switching. A one-line comment on stageCfg explaining the trade-off ("init = window cap so the full visible window renders in one frame") would save the next reader a git-blame trip.
session-scroll-position.spec.ts — Date.now() in session titles makes debugging flaky failures harder
If a test flakes, the session title is different on every run, so correlating logs across retries is painful. Consider a deterministic suffix (e.g. test.info().title or a counter) or at least logging the generated title before use.
session-send-stability.spec.ts — page.keyboard.type vs page.keyboard.insertText inconsistency
session-scroll-position.spec.ts uses insertText while session-send-stability.spec.ts uses type. They behave differently (type fires keydown/keyup, insertText does not). If the app specifically needs key events for the composer, type is correct; if not, standardise on one. Either way, a comment explaining the choice would help.
Astro-Han
left a comment
There was a problem hiding this comment.
Follow-up — items that couldn't be attached as line comments
session-send-stability.spec.ts — MAX_STABILITY_SAMPLES = 256 [P1]
This file is not in the PR diff, so a line comment isn't possible. The bound is unexplained; if a test generates more than 256 mutations the probe silently drops data. A brief comment on how this was chosen (e.g. "empirically ~120 samples for a 14-turn session switch") would help future maintainers.
session-send-stability.spec.ts — recordRemovedMount mutates unmountCount even when bounded [P2]
pushBounded silently drops samples once maxSamples is reached, but unmountCount keeps incrementing. This means unmountCount can exceed unmounts.length, which is surprising. Either cap unmountCount alongside the sample list or document the intentional divergence.
use-session-hash-scroll.ts — let clearing = false outside the reactive system [P2]
This line is outside the PR diff hunk, so a line comment isn't possible. A plain let flag is prone to stale-state bugs when Solid effects re-run out of order or are batched. Consider a signal or a ref object so the lifetime is explicit.
|
Follow-up outcomes for non-inline items:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/app/e2e/session/session-scroll-position.spec.ts`:
- Line 191: The URL assertion uses second.id directly inside a RegExp
(expect(page).toHaveURL(new RegExp(`/session/${second.id}(?:[?#]|$)`))) which is
unsafe because session IDs may contain regex metacharacters; escape second.id
before constructing the RegExp (e.g., replace all special regex chars with
backslash-escaped versions or use a utility like lodash/escapeRegExp) and then
build the RegExp with the escaped ID so the assertion is deterministic.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f3c657b7-988a-4af5-8fc2-5e3c1f502589
📒 Files selected for processing (3)
packages/app/e2e/session/session-scroll-position.spec.tspackages/app/src/pages/session/message-timeline.tsxpackages/app/src/pages/session/use-session-hash-scroll.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: unit-windows-app
- GitHub Check: unit-windows-opencode-server-tools
- GitHub Check: unit-windows-desktop
- GitHub Check: unit-windows-opencode-config-project
- GitHub Check: unit-windows-opencode-session
- GitHub Check: unit-opencode
- GitHub Check: typecheck
- GitHub Check: unit-desktop
- GitHub Check: unit-app
- GitHub Check: smoke-macos-arm64
- GitHub Check: analyze-js-ts
- GitHub Check: e2e-artifacts
🧰 Additional context used
📓 Path-based instructions (2)
packages/app/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (packages/app/AGENTS.md)
Always prefer
createStoreover multiplecreateSignalcalls in SolidJS
Files:
packages/app/src/pages/session/message-timeline.tsxpackages/app/src/pages/session/use-session-hash-scroll.tspackages/app/e2e/session/session-scroll-position.spec.ts
packages/app/e2e/**/*.spec.ts
📄 CodeRabbit inference engine (packages/app/e2e/AGENTS.md)
packages/app/e2e/**/*.spec.ts: Import test utilities from../fixturesinstead of@playwright/test
Test files should be named with the patternfeature-name.spec.ts
Use lowercase, descriptive test names (e.g., 'sidebar can be toggled')
Use camelCase for variable names in tests
Use SCREAMING_SNAKE_CASE for constants in tests
Use fixture-managed cleanup withwithSession(sdk, title, callback)for temporary sessions instead of callingsdk.session.delete(...)directly
Prefer theprojectfixture for tests that need a dedicated project with LLM mocking
Usedata-component,data-action, or semantic roles for selectors instead of CSS class names or IDs
UsemodKeyfrom utils for cross-platform keyboard shortcuts (Meta on Mac, Control on Linux/Windows)
In terminal tests, type through the browser usingrunTerminal()andwaitTerminalReady()instead of writing to the PTY through the SDK
Never use wall-clock waits likepage.waitForTimeout(...)to make a test pass
Wait on observable state withexpect(...),expect.poll(...), or existing helpers instead of assuming work is finished after an action
Use locator assertions liketoBeVisible(),toHaveCount(0), andtoHaveAttribute(...)for normal UI state verification
Useexpect.poll(...)for probing mock or backend state rather than transient DOM visibility
Prefer fluent helpers and drivers when they make intent obvious and reduce locator-heavy noise in tests
Use direct locators when the interaction is simple and a helper would not add clarity
When validating routing, assert against canonical or resolved workspace slugs using shared helpers from../actionsto account for Windows canonicalization
Test one feature per test file
Callproject.trackSession(sessionID, directory?)andproject.trackDirectory(directory)for any resources created outside the fixture so teardown can clean them up
Files:
packages/app/e2e/session/session-scroll-position.spec.ts
🧠 Learnings (19)
📓 Common learnings
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 247
File: packages/ui/src/components/message-part.tsx:1322-1324
Timestamp: 2026-04-26T16:34:57.130Z
Learning: In Astro-Han/pawwork (`packages/ui/src/components/message-part.tsx`), the `taskId` createMemo and `childSessionId` createMemo both intentionally read only from `partMetadata().sessionId` (populated post-execution), not from `input.task_id` / `input.subagent_session_id`. This has always been the case — the original code never read the input field either. Adding an `input.subagent_session_id` fallback would be a new capability, not a bug fix. Do NOT flag the absence of this fallback as a regression in PR `#247` or future PRs unless there is a concrete case where metadata is not populated.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/tool/agent.ts:23-27
Timestamp: 2026-04-28T04:38:11.771Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/tool/agent.ts`), the `subagent_session_id` field in the `Parameters` schema accepts any `Schema.String` rather than a branded `SessionID`. This is inherited upstream behavior (adopted in PR `#270`, an upstream-sync graft of upstream PR `#23244`). The fix — validating `subagent_session_id` as a `SessionID` brand up front so malformed/typo'd IDs fail explicitly rather than silently forking a new subagent session — is intentionally deferred to a follow-up PR or upstream report. Do NOT re-flag this as a blocking issue in PR `#270` or in future upstream-sync PRs that carry the same schema; flag it only in a PawWork-authored PR that touches `agent.ts` parameter validation.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 264
File: packages/opencode/src/session/prompt.ts:108-169
Timestamp: 2026-04-27T10:33:12.228Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/session/prompt.ts` and `packages/opencode/src/session/processor.ts`, PR `#264`), the loop-gate race condition between `buildLoopContext()` and `recordSyntheticBlock`/`recordSyntheticStop` is intentionally handled via idempotence guards (re-check sigKey presence / `hasStopped` inside the record helpers) rather than a full per-parent `Effect.Mutex`. Threading a `Map<MessageID, Mutex>` through the processor was considered too large a surface change for this edge case; the residual TOCTOU window only produces extra synthetic parts with no behavioral drift on the "turn ends" contract. A code comment documents the trade-off and points to a full-mutex follow-up if the race is observed in practice. Do NOT re-flag the absence of a per-parent mutex as a blocking issue in future reviews.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/patch/index.ts:337-346
Timestamp: 2026-04-28T04:38:05.946Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/patch/index.ts`), the BOM-transition surfacing gap — where `Bom.split` strips BOM before building `unified_diff`/`new_content` but `Bom.join` later re-attaches BOM on disk write, meaning BOM changes are not reflected in the diff payload — is intentionally deferred. PR `#270` is an upstream-sync graft; fixing the issue here would mix refactor + bugfix intents and drift the diff from the upstream baseline needed for clean future grafts. A dedicated follow-up PR (or upstream report) will address this. Do NOT re-flag the missing BOM-change surfacing in `ApplyPatchFileUpdate`/`ApplyPatchFileChange` as a blocking issue in PR `#270` or in future sync PRs that carry the same upstream baseline.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 193
File: packages/app/src/pages/layout/sidebar-items.tsx:102-107
Timestamp: 2026-04-23T15:26:07.250Z
Learning: In Astro-Han/pawwork (`packages/app/src/pages/layout/sidebar-items.tsx`), the `indicator()` function in `SessionRow` intentionally renders `props.leadingSlot` (the pin button) only as a fallback when no status indicator (running/permission/error/unseen) is active. When a higher-priority status wins the slot, the pin button is removed from the DOM — this is a deliberate design choice for the merged leading slot (`#150`). The keyboard unpin path is preserved via: (1) focusing the row anchor triggers `group-focus-within` which reveals the dots menu trigger, then Tab → Enter → "Unpin Session"; (2) the context menu (right-click / Shift+F10) exposes "Unpin Session". The "always render + CSS overlay" approach was considered but rejected due to z-index/pointer-events complexity; residual `...` slot behavior is tracked in `#192`. Do NOT flag the absence of the pin button from the DOM when a status is active as an accessibility regression.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/session/compaction.ts:169-191
Timestamp: 2026-04-28T05:36:22.128Z
Learning: In `packages/opencode/src/session/compaction.ts` (Astro-Han/pawwork), the internal helper `splitTurn` intentionally returns a raw `Effect.gen(...)` rather than being wrapped with `Effect.fnUntraced`. Converting `splitTurn` (and similar plain-Effect.gen internal helpers in this file, e.g. `turns`, `completedCompactions`, `buildPrompt`) to `Effect.fnUntraced` is a repo-wide convention sweep deferred to a dedicated follow-up PR. The same sweep covers `tool/tool.ts` and other helpers flagged in the same sync. Do NOT re-flag the absence of `Effect.fnUntraced` on `splitTurn` or other internal helpers in `compaction.ts` during upstream-sync PRs; flag it only in the dedicated convention-sweep PR.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/file/ripgrep.ts:311-328
Timestamp: 2026-04-28T05:36:18.200Z
Learning: In `packages/opencode/src/file/ripgrep.ts` (Astro-Han/pawwork, PR `#270`), the ripgrep binary download flow (`HttpClientRequest.get` → `fs.writeWithDirs` → `extract`) does NOT verify a pinned SHA-256 checksum. Adding per-platform hash verification (15.1.0 × 6 platforms) requires a dedicated manifest + `crypto.subtle.digest` step and is intentionally deferred to a follow-up PR. Existing mitigations: (1) `VERSION` is pinned to `"15.1.0"`, (2) downloads are over HTTPS from GitHub Releases, (3) PawWork production builds ship a bundled `rg` binary so this download path is a last-resort fallback (system PATH → cached → download). Do NOT re-flag the missing checksum verification in this file until the dedicated follow-up PR lands.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 206
File: packages/app/e2e/prompt/prompt-footer-focus.spec.ts:131-143
Timestamp: 2026-04-24T03:51:56.211Z
Learning: In Astro-Han/pawwork E2E tests (packages/app/e2e/fixtures.ts), `project.prompt(text)` internally calls `trackSession(next.sessionID, active.directory)` after the prompt submission is observed and the active session is resolved. Tests that obtain a `sessionID` from `project.prompt()` do NOT need to call `project.trackSession(sessionID)` manually — it is already registered for teardown. Calling it again would be duplicate ownership.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/tool/lsp.ts:23-32
Timestamp: 2026-04-28T07:28:14.317Z
Learning: In `packages/opencode/src/tool/lsp.ts` (Astro-Han/pawwork, PR `#270`), the `Parameters` schema requires `line` and `character` for all operations, including `workspaceSymbol` and `documentSymbol` which never use coordinates. This matches the upstream `dev:packages/opencode/src/tool/lsp.ts:23-32` baseline exactly — both fields are declared as required `Schema.Number` with `>= 1` checks. The fix (per-operation schema split, or making `line`/`character` optional with handler-side presence validation for operations that need them like `goToDefinition`/`findReferences`) is deferred to a follow-up PR or upstream report to avoid mixing refactor + bug-fix intents and drifting the diff from the upstream baseline. Do NOT re-flag the required coordinates on `workspaceSymbol`/`documentSymbol` in upstream-sync PRs; flag it only in a PawWork-authored PR that directly touches `lsp.ts` parameter validation.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 208
File: packages/app/src/components/prompt-input.tsx:1569-1611
Timestamp: 2026-04-24T05:39:58.329Z
Learning: In Astro-Han/pawwork `packages/app/src/components/prompt-input.tsx`, after the composer unification in PR `#208` (fixed in commit 5d810aa):
- `SendButton.disabled` does NOT gate on `store.mode !== "normal"`. Shell mode has a fully visible, clickable orange submit button that calls `handleSubmit` directly (same path as the Enter key in `handleKeyDown`). Do NOT suggest re-adding the mode gate.
- `SendButton` does NOT use the `buttons()` spring opacity animation (`style={buttons()}`). It is always fully visible regardless of mode.
- `WorkspaceChip` is gated on `props.homeMode && store.mode === "normal"` so it hides in shell mode (preventing it from appearing isolated/bright while neighboring controls fade).
- The left-side chip group (`aria-hidden={store.mode !== "normal"}`) covers attach/model/variant/workspace controls only; `SendButton` remains in a separate right-side sibling div.
📚 Learning: 2026-04-26T16:34:57.130Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 247
File: packages/ui/src/components/message-part.tsx:1322-1324
Timestamp: 2026-04-26T16:34:57.130Z
Learning: In Astro-Han/pawwork (`packages/ui/src/components/message-part.tsx`), the `taskId` createMemo and `childSessionId` createMemo both intentionally read only from `partMetadata().sessionId` (populated post-execution), not from `input.task_id` / `input.subagent_session_id`. This has always been the case — the original code never read the input field either. Adding an `input.subagent_session_id` fallback would be a new capability, not a bug fix. Do NOT flag the absence of this fallback as a regression in PR `#247` or future PRs unless there is a concrete case where metadata is not populated.
Applied to files:
packages/app/src/pages/session/message-timeline.tsx
📚 Learning: 2026-04-23T07:23:23.849Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 180
File: packages/app/src/components/session/session-new-view.tsx:13-18
Timestamp: 2026-04-23T07:23:23.849Z
Learning: In pawwork (Astro-Han/pawwork), prefer using `createStore` instead of multiple `createSignal` calls only when the signals represent **coupled** object state that is updated together (i.e., there is at least one shared batch-update site where the state is changed in the same transaction). If the state fields are **independent** and are mutated by separate handlers (e.g., one handler updates only `selectedSkill` while another updates only `mode`), keep them as individual `createSignal` calls—using `createStore` for truly independent fields adds boilerplate without behavioral benefit.
Applied to files:
packages/app/src/pages/session/message-timeline.tsxpackages/app/src/pages/session/use-session-hash-scroll.tspackages/app/e2e/session/session-scroll-position.spec.ts
📚 Learning: 2026-04-23T15:10:21.635Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 191
File: packages/app/src/components/session/pawwork-skill-meta.ts:38-39
Timestamp: 2026-04-23T15:10:21.635Z
Learning: This repo configures Tailwind v4 with `--color-*: initial`, which effectively breaks standard Tailwind palette utilities (e.g., `text-violet-500` can resolve to no CSS variable and render as a no-op/black). For brand/accent colors that are not backed by semantic design tokens, use inline styles with the exact hex value (e.g., `style={{ color: '#8B5FBF' }}` / `homeIconStyle: { color: '#8B5FBF' }`) and add a short comment explaining that Tailwind palette utilities won’t work due to the `--color-*: initial` setup. Do not suggest replacing these inline hex colors with Tailwind palette classes anywhere in this repo.
Applied to files:
packages/app/src/pages/session/message-timeline.tsxpackages/app/src/pages/session/use-session-hash-scroll.ts
📚 Learning: 2026-04-27T10:33:12.228Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 264
File: packages/opencode/src/session/prompt.ts:108-169
Timestamp: 2026-04-27T10:33:12.228Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/session/prompt.ts` and `packages/opencode/src/session/processor.ts`, PR `#264`), the loop-gate race condition between `buildLoopContext()` and `recordSyntheticBlock`/`recordSyntheticStop` is intentionally handled via idempotence guards (re-check sigKey presence / `hasStopped` inside the record helpers) rather than a full per-parent `Effect.Mutex`. Threading a `Map<MessageID, Mutex>` through the processor was considered too large a surface change for this edge case; the residual TOCTOU window only produces extra synthetic parts with no behavioral drift on the "turn ends" contract. A code comment documents the trade-off and points to a full-mutex follow-up if the race is observed in practice. Do NOT re-flag the absence of a per-parent mutex as a blocking issue in future reviews.
Applied to files:
packages/app/src/pages/session/use-session-hash-scroll.ts
📚 Learning: 2026-04-24T03:51:54.050Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 206
File: packages/app/e2e/prompt/prompt-footer-focus.spec.ts:131-143
Timestamp: 2026-04-24T03:51:54.050Z
Learning: In Astro-Han/pawwork E2E tests under packages/app/e2e, do not manually call `project.trackSession(sessionID)` when you obtain a `sessionID` via `project.prompt(text)`. The `project.prompt()` implementation already registers `trackSession(next.sessionID, active.directory)` automatically after the prompt submission is observed and the active session is resolved, so calling `project.trackSession(sessionID)` again will create duplicate session ownership/teardown handling.
Applied to files:
packages/app/e2e/session/session-scroll-position.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Use fixture-managed cleanup with `withSession(sdk, title, callback)` for temporary sessions instead of calling `sdk.session.delete(...)` directly
Applied to files:
packages/app/e2e/session/session-scroll-position.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Call `project.trackSession(sessionID, directory?)` and `project.trackDirectory(directory)` for any resources created outside the fixture so teardown can clean them up
Applied to files:
packages/app/e2e/session/session-scroll-position.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Use `expect.poll(...)` for probing mock or backend state rather than transient DOM visibility
Applied to files:
packages/app/e2e/session/session-scroll-position.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Import test utilities from `../fixtures` instead of `playwright/test`
Applied to files:
packages/app/e2e/session/session-scroll-position.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Test one feature per test file
Applied to files:
packages/app/e2e/session/session-scroll-position.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Use lowercase, descriptive test names (e.g., 'sidebar can be toggled')
Applied to files:
packages/app/e2e/session/session-scroll-position.spec.ts
📚 Learning: 2026-04-28T05:36:25.456Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/test/tool/grep.test.ts:16-22
Timestamp: 2026-04-28T05:36:25.456Z
Learning: In `packages/opencode/test/tool/grep.test.ts` (Astro-Han/pawwork, PR `#270`), the test suite intentionally uses a manual `ManagedRuntime.make(Layer.mergeAll(...))` setup and raw `bun:test` test cases rather than the `testEffect(...)` harness. The migration to `testEffect` + `it.live(...)` + `provideTmpdirInstance(...)` is a whole-file rewrite deferred to a dedicated sweep PR that will migrate all tool tests still on the manual ManagedRuntime pattern together. Do NOT re-flag the ManagedRuntime setup in this file as needing harness migration until that sweep PR lands.
Applied to files:
packages/app/e2e/session/session-scroll-position.spec.ts
📚 Learning: 2026-04-27T12:59:49.844Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 264
File: packages/opencode/test/session/prompt-effect.test.ts:0-0
Timestamp: 2026-04-27T12:59:49.844Z
Learning: In `packages/opencode/test/session/prompt-effect.test.ts` and `packages/opencode/src/session/diagnostics.ts` (PR `#264`), the recovery reminder copy differs between signature kinds: the input-repeat variant says "repeated the same tool input 3 times" (uses a literal count), while the target-repeat variant says "failed against the same target multiple times" (uses "multiple times" with no count). Assertions that check for injected reminder text in LLM inputs must accept both phrasings when a scenario produces both `input:` and `target:` signatures (e.g., `read` tool with a `filePath` parameter). Do NOT narrow the assertion to only the input-variant phrasing.
Applied to files:
packages/app/e2e/session/session-scroll-position.spec.ts
📚 Learning: 2026-04-20T14:36:04.113Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/app/e2e/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:04.113Z
Learning: Applies to packages/app/e2e/**/*.spec.ts : Never use wall-clock waits like `page.waitForTimeout(...)` to make a test pass
Applied to files:
packages/app/e2e/session/session-scroll-position.spec.ts
📚 Learning: 2026-04-20T14:36:21.288Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:21.288Z
Learning: Applies to packages/opencode/**/*.ts : Prefer `DateTime.nowAsDate` over `new Date(yield* Clock.currentTimeMillis)` when you need a `Date` in Effect code
Applied to files:
packages/app/e2e/session/session-scroll-position.spec.ts
📚 Learning: 2026-04-23T15:25:27.182Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 193
File: packages/app/e2e/sidebar/sidebar-leading-slot.spec.ts:5-55
Timestamp: 2026-04-23T15:25:27.182Z
Learning: In Astro-Han/pawwork E2E tests (e.g., *.spec.ts under packages/app/e2e), reaching a real "running" session state is not achievable with the bare `sdk` fixture. Use the `project` fixture (to bootstrap the model) and orchestrate the transition with `llm.wait(1)`; even if you set `agent: "build"` and a `system` prompt via `sdk.session.promptAsync`, the current test infrastructure does not trigger an actual LLM call, so it won’t simulate "running" cheaply. Review any attempt to mock/force "running" using only `sdk` as likely ineffective unless it also uses `project` + `llm.wait(1)`.
Applied to files:
packages/app/e2e/session/session-scroll-position.spec.ts
📚 Learning: 2026-04-24T00:02:50.599Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 203
File: packages/app/e2e/sidebar/sidebar-session-links.spec.ts:34-55
Timestamp: 2026-04-24T00:02:50.599Z
Learning: For Astro-Han/pawwork E2E tests under packages/app/e2e/**/*.spec.ts, do not call project.trackDirectory() or project.trackSession() before project.open() has run. The project fixture throws until open() initializes internal state. Use this ordering pattern: (1) call project.trackSession(sessionID) inside the beforeGoto callback (where state is already available), (2) call project.trackDirectory(directory) and any cross-workspace tracking like project.trackSession(id, directory) immediately after project.open() returns, and (3) if you create any resources before open() that cannot yet be tracked via the fixture, ensure you clean them up explicitly in finally blocks (e.g., cleanupSession / cleanupTestProject).
Applied to files:
packages/app/e2e/session/session-scroll-position.spec.ts
📚 Learning: 2026-04-24T05:48:36.205Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 208
File: packages/app/e2e/app/composer-parity.spec.ts:0-0
Timestamp: 2026-04-24T05:48:36.205Z
Learning: In E2E parity tests, prefer using the existing `[data-action]` coverage when asserting UI parity. For elements whose trigger props set `data-action` (e.g., `data-action="prompt-model"` and `data-action="prompt-model-variant"` on prompt input chip triggers), you generally do not need to add separate assertions driven by `[data-component]` for parity. Avoid duplicating component-specific queries when the `[data-action]` selector sweep already includes the elements; any extra unioning of selectors should be treated as optional belt-and-suspenders rather than required.
Applied to files:
packages/app/e2e/session/session-scroll-position.spec.ts
🪛 ast-grep (0.42.1)
packages/app/e2e/session/session-scroll-position.spec.ts
[warning] 190-190: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(/session/${second.id}(?:[?#]|$))
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (3)
packages/app/e2e/session/session-scroll-position.spec.ts (1)
38-44: Avoidthrowinsidepage.evaluate.Returning a sentinel and asserting in the test body keeps the failure anchored to the test line instead of a generic browser-context error.
Proposed fix
async function scrollTimelineToBottom(page: Page) { - await page.evaluate((turnListSelector) => { + const found = await page.evaluate((turnListSelector) => { const list = document.querySelector(turnListSelector) const viewport = list?.closest(".scroll-view__viewport") - if (!(viewport instanceof HTMLElement)) throw new Error("session timeline viewport not found") + if (!(viewport instanceof HTMLElement)) return false viewport.scrollTop = viewport.scrollHeight + return true }, sessionTurnListSelector) + expect(found, "session timeline viewport should exist").toBe(true) }packages/app/src/pages/session/message-timeline.tsx (1)
381-382: Looks good.The
init: 10cap matches the new staging behavior, and the inline note explains the trade-off clearly.packages/app/src/pages/session/use-session-hash-scroll.ts (1)
30-60: Looks good — the stale-hash guard is now keyed to the exact hash being cleared.This closes the re-apply race without changing the public behavior of hash navigation.
Also applies to: 145-200
|
Non-inline CodeRabbit outcomes:
|
Summary
Why
Two session timeline issues could make the UI feel unstable:
Related Issue
No linked issue. Reported directly during debugging.
How To Verify
List commands:
(cd packages/app && bun playwright test e2e/session/session-scroll-position.spec.ts)
(cd packages/app && bun playwright test e2e/session/session-send-stability.spec.ts)
(cd packages/app && bun run typecheck)
Screenshots or Recordings
Not attached. This is interaction stability behavior covered by E2E checks.
Checklist
Summary by CodeRabbit
Tests
Bug Fixes