perf: add browse lazy loading and archive stale workflows - #25
Conversation
|
Warning Review limit reached
More reviews will be available in 29 minutes and 50 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughI can’t reliably rebuild the full review stack artifact with every rangeId placed exactly once within this chat (the PR contains hundreds of rangeIds and the validator requires exact one-to-one mapping). Please run the provided reviewer tool locally or supply a smaller set of ranges (≤ ~60) so I can produce a valid review stack artifact that passes validation. ✨ Finishing Touches🧪 Generate unit tests (beta)
|
feat: lazy-load memory browse and Flashbacks
|
review @coderabbitai @codex |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get PR details and list changed files
gh pr view 25 --repo hauntedfail/Trauma --json title,body,files,additions,deletions,changedFiles
echo "=== CHANGED FILES ==="
gh pr diff 25 --repo hauntedfail/Trauma --name-only
echo "=== FULL DIFF ==="
gh pr diff 25 --repo hauntedfail/TraumaLength of output: 233049
PR Review
|
| Severity | Issue | File |
|---|---|---|
| PR scope/title mismatch | — | |
| 🔴 | No max-iteration guard in flashback-filter pagination loop | src/server/memories/browse.ts |
| 🔴 | normalizePageRequestLimit missing upper cap |
src/server/memories/browse.ts |
| 🟡 | Reader flashback load path needs explicit verification after aggregate refactor | src/server/reader/page-data.ts |
| 🟡 | lazyAllFlashbacks undefined-for-both-states semantic gap |
src/components/reader/MemoryReader.tsx |
| 🟡 | Duplicate normalization helpers | repositories.ts, browse.ts |
| 🟢 | Unreachable SSR branch in scheduleFocusTabButton |
src/components/memories/MemoryBrowse.tsx |
![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: 7889d42b27
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
tests/components/memory-browse-actions.test.ts (1)
188-224: 🏗️ Heavy liftPrefer behavior tests over source-text checks for the pagination wiring.
These assertions pass as long as the helper names remain in the file, even if the actual pagination/reset/revalidation behavior breaks. Mocking the loaders and asserting rendered rows or revalidation calls would give this feature real regression coverage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/components/memory-browse-actions.test.ts` around lines 188 - 224, The tests currently assert presence of helper names in source text which is brittle; instead mock the relevant loader and revalidation functions and assert runtime behavior: mount the browsing UI and stub getBrowseMemoryPage/createInitialBrowseMemoryPageRequest/createNextBrowseMemoryPageRequest to return paged results and verify rendered rows and that getBrowseMemoryPage is called for pages, stub getBrowseFlashbacksForMemories and assert flashbacks are fetched only for visibleMemoryIds and passed to flashbacksByMemoryId, change the route/query and assert isSameBrowseQuery triggers setAdditionalPages([])/setRemovedMemoryIds(...) and setLoadNextPageError(""), simulate deleting a memory and assert removedMemoryIds keeps it filtered across loaded pages, and on add-memory success stub parseBrowseQuery(location.search) and assert revalidateBrowseMemoryFirstPage(query()) and revalidateBrowseTaxonomy are called (and revalidateBrowseMemories is not); use the symbols from the diff (e.g., getBrowseMemoryPage, getBrowseFlashbacksForMemories, visibleMemoryIds, removedMemoryIds, revalidateBrowseMemoryFirstPage, revalidateBrowseTaxonomy) to locate and replace the source-text assertions with behavior-driven mocks and rendered-output assertions.src/server/db/schema.ts (1)
93-95: ⚡ Quick winDrop the redundant
created_atindex.
memories_created_at_id_idxalready coverscreatedAt-only scans/orderings, so keepingmemories_created_at_idxadds extra index maintenance on every write without opening a new access path.♻️ Proposed fix
- index("memories_created_at_idx").on(table.createdAt), index("memories_created_at_id_idx").on(table.createdAt, table.id),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/db/schema.ts` around lines 93 - 95, Remove the redundant single-column index declaration index("memories_created_at_idx").on(table.createdAt) from the schema since index("memories_created_at_id_idx").on(table.createdAt, table.id) already covers createdAt-only scans and ordering; locate the index declarations near the memories_* indexes (the entries referencing index("memories_url_idx"), index("memories_created_at_idx"), and index("memories_created_at_id_idx")) and delete the line that defines memories_created_at_idx so only the composite createdAt+id index remains.src/server/db/repositories.ts (1)
683-685: ⚡ Quick winAdd a stable secondary sort to the non-paginated browse queries.
The new page/recent browse paths break ties with
(createdAt, id), but these two queries still sort only bycreatedAt. Rows created in the same millisecond can flip order between calls, which makes the older browse paths inconsistent with the paginated contract.♻️ Proposed fix
- const rows = await selectFlashbackBrowseRows(db) - .orderBy(desc(schema.flashbacks.createdAt)); + const rows = await selectFlashbackBrowseRows(db).orderBy( + desc(schema.flashbacks.createdAt), + desc(schema.flashbacks.id), + ); ... - orderBy: [desc(schema.memories.createdAt)], + orderBy: [desc(schema.memories.createdAt), desc(schema.memories.id)],Also applies to: 874-875
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/db/repositories.ts` around lines 683 - 685, The non-paginated browse queries using selectFlashbackBrowseRows currently only orderBy(desc(schema.flashbacks.createdAt)) causing unstable ties; update the orderBy calls (e.g., in selectFlashbackBrowseRows usage at the shown spot and the other occurrence around lines 874-875) to add a stable secondary sort on the primary key (schema.flashbacks.id) so the sort becomes deterministic (tie-break createdAt with id using the same direction as the createdAt ordering).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/memories/MemoryBrowse.tsx`:
- Around line 145-156: The catch block in loadNextPage (where
getBrowseMemoryPage is called) currently sets setLoadNextPageError but then
rethrows the error, causing unhandled promise rejections because callers invoke
loadNextPage() as void; remove the throw error so the function swallows the
handled failure and only reports it via setLoadNextPageError (optionally log the
error instead of rethrowing) — locate the try/catch around getBrowseMemoryPage
and remove the "throw error" statement in the catch that follows
setLoadNextPageError.
- Around line 128-129: The IntersectionObserver is only created/attached inside
onMount() when loadMoreSentinel is already set, so on first async render the
sentinel div (rendered inside Show when visibleMemories() > 0) may not be
mounted and infinite-scroll never activates; to fix, initialize the
IntersectionObserver in onMount() (or lazily create it) but attach/observe
inside the sentinel ref callback whenever loadMoreSentinel becomes non-null and
disconnect it when the ref becomes null or on cleanup; update the ref handling
for loadMoreSentinel and the onCleanup/unmount logic so
observer.observe(loadMoreSentinel) is called only after the element exists and
observer.disconnect() is invoked when the element is removed or component
unmounts, referencing loadMoreSentinel, onMount(), the sentinel ref assignment,
and the IntersectionObserver instance.
In `@src/components/shell/AppShell.tsx`:
- Around line 166-173: The current lazy-load predicate showRightRailFlashbacks
uses rightRailContent() which is undefined on first render and triggers
flashbacks prematurely; change showRightRailFlashbacks (and thus the flashbacks
resource) to derive from shell-owned route state or an explicit page flag
instead of a child-provided value — e.g., replace the rightRailContent() check
with a shell signal or route-derived helper (use activePath() or a new
isReaderPage/isRightRailManaged signal owned by AppShell) so that flashbacks
only call getRecentFlashbackBrowseRows(5) when the shell knows the right rail
should be blank and not while MemoryReader is still installing its custom rail.
In `@src/server/flashbacks/browse.ts`:
- Line 22: The backfill is limited to RECENT_FLASHBACK_BACKFILL_CANDIDATE_LIMIT
= 100 which, combined with normalizeFlashbackLimit(), prevents fetching older
renderable rows when some of the top 100 are stale; update the logic used by the
backfill query (the code that uses RECENT_FLASHBACK_BACKFILL_CANDIDATE_LIMIT in
browse/recent flashback queries) to widen the candidate window—either increase
RECENT_FLASHBACK_BACKFILL_CANDIDATE_LIMIT (e.g., 500) or compute it relative to
the requested limit (e.g., Math.max(100, normalizeFlashbackLimit(requestedLimit)
* 3)) so the follow-up backfill can find older renderable rows; ensure the same
change is applied to the other backfill usage around the browse recent-backfill
logic.
In `@src/server/memories/browse.ts`:
- Around line 71-81: Normalize the browse-page limit once before the branching
logic instead of letting different paths use different values: compute a
clampedLimit (apply the same clamp logic used by the renderable-flashback path
and fixture-mode lower bound rules to request.limit/TRAUMA_BROWSE_FIXTURES) and
inject that normalized value into toMemoryBrowsePageRepositoryInput (or replace
request.limit in the repositoryInput) and pass clampedLimit into both
listBrowseMemoryPageWithRenderableFlashbackFilters and
connection.repositories.memories.listForBrowsePage calls so both branches use
the identical page size; apply the same change to the other browse-page sites
that use the same pattern (the blocks around the other occurrences of this
branching logic).
In `@src/server/reader/page-data.ts`:
- Around line 348-358: The current loop in
listCompleteTranslationRecordsForMemory handling (involving
listCompleteTranslationRecordsForMemory, translationsByLanguage, job.sourceHash,
isSupportedLanguageCode, job.langCode) blindly sets the first matching job per
language and skips later candidates—change it to collect all matching jobs per
language (e.g., map langCode -> array of jobs) rather than calling
translationsByLanguage.set(...) on first sight; after collection,
deterministically pick a single winner per language by applying a comparator
that prefers jobs that match input.sourceHash, then jobs with job.current ===
true (or equivalent freshness/readable flag), then most recent updated/created
timestamp, then a stable tie-breaker (e.g., id) and store that winner in
translationsByLanguage. Ensure you still call isSupportedLanguageCode and keep
filtering by sourceHash as appropriate while collecting candidates.
In `@tests/server/db/repositories.test.ts`:
- Around line 1033-1075: The test uses the unsupported readState "both" when
calling page({ readState: "both" }) which masks the real "all" branch; replace
that call with page({ readState: "all" }) and update the corresponding assertion
for bothIds to reflect the expected ids from the "all" branch (or alternatively
add a new test that calls page({ readState: "both" }) and asserts it is rejected
as invalid). Locate the page() invocations and the bothPage/bothIds assertion in
repositories.test.ts and make the change so the exercised path covers the
supported "all" readState (or add the separate invalid-input test that asserts
proper error handling).
---
Nitpick comments:
In `@src/server/db/repositories.ts`:
- Around line 683-685: The non-paginated browse queries using
selectFlashbackBrowseRows currently only
orderBy(desc(schema.flashbacks.createdAt)) causing unstable ties; update the
orderBy calls (e.g., in selectFlashbackBrowseRows usage at the shown spot and
the other occurrence around lines 874-875) to add a stable secondary sort on the
primary key (schema.flashbacks.id) so the sort becomes deterministic (tie-break
createdAt with id using the same direction as the createdAt ordering).
In `@src/server/db/schema.ts`:
- Around line 93-95: Remove the redundant single-column index declaration
index("memories_created_at_idx").on(table.createdAt) from the schema since
index("memories_created_at_id_idx").on(table.createdAt, table.id) already covers
createdAt-only scans and ordering; locate the index declarations near the
memories_* indexes (the entries referencing index("memories_url_idx"),
index("memories_created_at_idx"), and index("memories_created_at_id_idx")) and
delete the line that defines memories_created_at_idx so only the composite
createdAt+id index remains.
In `@tests/components/memory-browse-actions.test.ts`:
- Around line 188-224: The tests currently assert presence of helper names in
source text which is brittle; instead mock the relevant loader and revalidation
functions and assert runtime behavior: mount the browsing UI and stub
getBrowseMemoryPage/createInitialBrowseMemoryPageRequest/createNextBrowseMemoryPageRequest
to return paged results and verify rendered rows and that getBrowseMemoryPage is
called for pages, stub getBrowseFlashbacksForMemories and assert flashbacks are
fetched only for visibleMemoryIds and passed to flashbacksByMemoryId, change the
route/query and assert isSameBrowseQuery triggers
setAdditionalPages([])/setRemovedMemoryIds(...) and setLoadNextPageError(""),
simulate deleting a memory and assert removedMemoryIds keeps it filtered across
loaded pages, and on add-memory success stub parseBrowseQuery(location.search)
and assert revalidateBrowseMemoryFirstPage(query()) and revalidateBrowseTaxonomy
are called (and revalidateBrowseMemories is not); use the symbols from the diff
(e.g., getBrowseMemoryPage, getBrowseFlashbacksForMemories, visibleMemoryIds,
removedMemoryIds, revalidateBrowseMemoryFirstPage, revalidateBrowseTaxonomy) to
locate and replace the source-text assertions with behavior-driven mocks and
rendered-output assertions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: cdac5d8e-6b61-4f08-8527-8fa3319fa2e1
📒 Files selected for processing (101)
docs/workflows/README.mddocs/workflows/archive/task-10-runtime-dev-server-stabilization.mddocs/workflows/archive/task-19-codex-translation-auth-repair.mddocs/workflows/archive/task-19-codex-translation-model-controls.mddocs/workflows/archive/task-19-codex-translation-protocol-repair.mddocs/workflows/archive/task-19-codex-translation-reader-projections.mddocs/workflows/archive/task-19-codex-translation-reader-projections/01-projection-contract-and-persistence.mddocs/workflows/archive/task-19-codex-translation-reader-projections/02-translation-projection-generation.mddocs/workflows/archive/task-19-codex-translation-reader-projections/03-translated-reader-projection-rendering.mddocs/workflows/archive/task-19-codex-translation-reader-projections/04-cross-variant-flashback-toggle.mddocs/workflows/archive/task-19-codex-translation-reader-projections/05-cross-variant-moment-toggle.mddocs/workflows/archive/task-19-codex-translation-reader-projections/06-integration-docs-and-verification.mddocs/workflows/archive/task-19-codex-translation-review.mddocs/workflows/archive/task-19-codex-translation-segment-reassembly.mddocs/workflows/archive/task-19-codex-translation-segment-reassembly/01-regression-fixtures-and-library-decision.mddocs/workflows/archive/task-19-codex-translation-segment-reassembly/02-parser-adapter.mddocs/workflows/archive/task-19-codex-translation-segment-reassembly/03-segment-manifest-and-reassembly.mddocs/workflows/archive/task-19-codex-translation-segment-reassembly/04-structure-fingerprint-validation.mddocs/workflows/archive/task-19-codex-translation-segment-reassembly/05-prompt-schema-and-policy.mddocs/workflows/archive/task-19-codex-translation-segment-reassembly/06-chunker-runner-and-stitching.mddocs/workflows/archive/task-19-codex-translation-segment-reassembly/07-workflow-contracts-and-docs.mddocs/workflows/archive/task-19-codex-translation-segment-reassembly/08-end-to-end-verification.mddocs/workflows/archive/task-19-codex-translation-validation-feedback-repair.mddocs/workflows/archive/task-19-codex-translation-variant-local-flashbacks.mddocs/workflows/archive/task-19-codex-translation-variant-local-flashbacks/01-contract-and-schema-migration.mddocs/workflows/archive/task-19-codex-translation-variant-local-flashbacks/02-repository-and-variant-domain.mddocs/workflows/archive/task-19-codex-translation-variant-local-flashbacks/03-toggle-service-and-api-route.mddocs/workflows/archive/task-19-codex-translation-variant-local-flashbacks/04-reader-rendering-and-current-variant-state.mddocs/workflows/archive/task-19-codex-translation-variant-local-flashbacks/05-browse-route-delete-and-export.mddocs/workflows/archive/task-19-codex-translation-variant-local-flashbacks/06-docs-cleanup-and-verification.mddocs/workflows/archive/task-19-codex-translation.mddocs/workflows/archive/task-19-codex-translation/00-execution-contracts.mddocs/workflows/archive/task-19-codex-translation/01-requirements-and-architecture-finalization.mddocs/workflows/archive/task-19-codex-translation/02-sqlite-schema-and-migration-design.mddocs/workflows/archive/task-19-codex-translation/03-translation-job-state-machine.mddocs/workflows/archive/task-19-codex-translation/04-markdown-block-manifest-and-chunker.mddocs/workflows/archive/task-19-codex-translation/05-codex-app-server-integration.mddocs/workflows/archive/task-19-codex-translation/06-codex-auth-and-device-code-setup-flow.mddocs/workflows/archive/task-19-codex-translation/07-streaming-event-bridge-to-frontend.mddocs/workflows/archive/task-19-codex-translation/08-chunk-translation-prompt-and-output-schema.mddocs/workflows/archive/task-19-codex-translation/09-chunk-validation-and-retry-logic.mddocs/workflows/archive/task-19-codex-translation/10-stitching-and-atomic-commit.mddocs/workflows/archive/task-19-codex-translation/11-sqlite-cleanup-and-purge-policy.mddocs/workflows/archive/task-19-codex-translation/12-frontend-translation-controls-and-progress-ui.mddocs/workflows/archive/task-19-codex-translation/13-reader-render-integration-for-translated-content.mddocs/workflows/archive/task-19-codex-translation/14-translation-skill-definition.mddocs/workflows/archive/task-19-codex-translation/15-error-handling-and-cancellation.mddocs/workflows/archive/task-19-codex-translation/16-test-plan-and-fixtures.mddocs/workflows/archive/task-19-codex-translation/17-end-to-end-validation-with-long-paper-fixture.mddocs/workflows/archive/task-19-codex-translation/README.mddocs/workflows/archive/task-19-codex-translation/contracts/01-architecture-and-ownership.mddocs/workflows/archive/task-19-codex-translation/contracts/02-types-state-and-settings.mddocs/workflows/archive/task-19-codex-translation/contracts/03-sqlite-and-repositories.mddocs/workflows/archive/task-19-codex-translation/contracts/04-api-and-sse.mddocs/workflows/archive/task-19-codex-translation/contracts/05-markdown-chunking.mddocs/workflows/archive/task-19-codex-translation/contracts/06-codex-prompt-and-validation.mddocs/workflows/archive/task-19-codex-translation/contracts/07-atomic-commit-purge-recovery.mddocs/workflows/archive/task-19-codex-translation/contracts/README.mddocs/workflows/task-11-test-suite-health-refactor.mddocs/workflows/task-15-refactor-wave-integration.mddocs/workflows/task-20-lazy-loading-performance/01-browse-query-and-page-contract.mddocs/workflows/task-20-lazy-loading-performance/02-sqlite-repository-pagination.mddocs/workflows/task-20-lazy-loading-performance/03-browse-loader-contract.mddocs/workflows/task-20-lazy-loading-performance/04-memories-infinite-scroll-ui.mddocs/workflows/task-20-lazy-loading-performance/05-lazy-flashback-browse-data.mddocs/workflows/task-20-lazy-loading-performance/06-reader-lazy-flashback-tab.mddocs/workflows/task-20-lazy-loading-performance/07-integration-verification-and-handoff.mddocs/workflows/task-20-lazy-loading-performance/README.mddrizzle/0015_memory_browse_pagination.sqldrizzle/meta/_journal.jsone2e/browse-shell.spec.tssrc/components/flashbacks/flashbacks-loader.tssrc/components/memories/AddMemoryForm.tsxsrc/components/memories/MemoryBrowse.tsxsrc/components/memories/browse-data.tssrc/components/memories/browse-fixtures.tssrc/components/memories/browse-loader.tssrc/components/reader/MemoryReader.tsxsrc/components/shell/AppShell.tsxsrc/server/db/bundled-migrations.tssrc/server/db/repositories.tssrc/server/db/schema.tssrc/server/flashbacks/browse.tssrc/server/memories/browse.tssrc/server/reader/page-data.tssrc/server/translation/current-translation.tstests/components/app-shell.test.tstests/components/browse-loader.test.tstests/components/flashbacks-loader.test.tstests/components/memory-browse-actions.test.tstests/components/memory-reader-actions.test.tstests/components/reader-flashback-tabs.test.tstests/memories/browse-data.test.tstests/server/backup/backup-environment.test.tstests/server/browse-loaders.test.tstests/server/db/repositories.test.tstests/server/db/schema.test.tstests/server/flashbacks/repository.test.tstests/server/memories/browse.test.tstests/server/reader/page-data.test.tstests/server/translation/source-and-current.test.ts
nonnil
changed the title
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
review @copilot |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/references/design-system/reader-and-content.md (1)
73-73: 💤 Low valueConsider hyphenating compound modifier.
Style guides typically hyphenate compound modifiers that precede nouns. "one-third down" is clearer as a single adjectival phrase.
✏️ Suggested style fix
-- `MemoryReader` observes the rendered section headings (`[data-reader-section-anchor]`) inside the reading column and resolves the - active heading against a reading line one third down the viewport. + active heading against a reading line one-third down the viewport.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/references/design-system/reader-and-content.md` at line 73, Replace the unhyphenated compound modifier "one third down" with the hyphenated form "one-third down" in the sentence "active heading against a reading line one third down the viewport" so the phrase reads "active heading against a reading line one-third down the viewport" to follow compound-modifier style guidance.tests/components/reader-toc-scroll-spy.test.ts (1)
12-16: ⚡ Quick winConsider the maintenance cost of source-reading tests.
Reading implementation files directly creates brittle tests that break on file renames, refactoring, or variable changes. While this pattern can enforce specific contracts (e.g., ensuring the accessor pattern at lines 92-97 or design tokens at line 107), it couples tests to implementation details.
Consider supplementing these checks with runtime component tests that verify the same contracts through behavior (e.g., mounting
MemoryReader, scrolling, and asserting the TOC receives updates without remounting). Runtime tests are more resilient to refactoring while still catching breakage.If you prefer to keep the source-reading pattern for explicit contract enforcement, document this trade-off so future maintainers understand the intentional coupling.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/components/reader-toc-scroll-spy.test.ts` around lines 12 - 16, Tests currently read implementation files directly via readerSource and tailwindSource (coupling to MemoryReader internals); replace or supplement these brittle source-reading assertions with runtime component tests that mount MemoryReader, simulate scrolling, and assert the TOC component/state updates without remounting (verify visible TOC entry changes and any exposed callbacks/props), or if you intentionally need source-level assertions, add a short doc comment in the test explaining the trade-off and why readerSource/tailwindSource checks are required so future maintainers know the coupling is intentional.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@docs/references/design-system/reader-and-content.md`:
- Line 73: Replace the unhyphenated compound modifier "one third down" with the
hyphenated form "one-third down" in the sentence "active heading against a
reading line one third down the viewport" so the phrase reads "active heading
against a reading line one-third down the viewport" to follow compound-modifier
style guidance.
In `@tests/components/reader-toc-scroll-spy.test.ts`:
- Around line 12-16: Tests currently read implementation files directly via
readerSource and tailwindSource (coupling to MemoryReader internals); replace or
supplement these brittle source-reading assertions with runtime component tests
that mount MemoryReader, simulate scrolling, and assert the TOC component/state
updates without remounting (verify visible TOC entry changes and any exposed
callbacks/props), or if you intentionally need source-level assertions, add a
short doc comment in the test explaining the trade-off and why
readerSource/tailwindSource checks are required so future maintainers know the
coupling is intentional.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c15bacaf-fe0b-499e-8be6-c474bacc5229
📒 Files selected for processing (14)
docs/architecture/ui-and-routing.mddocs/references/design-system/reader-and-content.mddocs/workflows/README.mddocs/workflows/task-23-reader-toc-reading-progress/01-active-range-model.mddocs/workflows/task-23-reader-toc-reading-progress/02-reader-scroll-spy-wiring.mddocs/workflows/task-23-reader-toc-reading-progress/03-toc-reading-range-visualization.mddocs/workflows/task-23-reader-toc-reading-progress/04-docs-verification-handoff.mddocs/workflows/task-23-reader-toc-reading-progress/README.mdsrc/components/reader/MemoryReader.tsxsrc/components/reader/toc-reading-range.tssrc/components/reader/toc-scroll-spy.tssrc/styles/tailwind.csstests/components/reader-toc-reading-range.test.tstests/components/reader-toc-scroll-spy.test.ts
✅ Files skipped from review due to trivial changes (5)
- docs/workflows/task-23-reader-toc-reading-progress/04-docs-verification-handoff.md
- docs/workflows/task-23-reader-toc-reading-progress/03-toc-reading-range-visualization.md
- docs/workflows/task-23-reader-toc-reading-progress/02-reader-scroll-spy-wiring.md
- docs/architecture/ui-and-routing.md
- docs/workflows/README.md
- track every section currently rendered on screen instead of locking to one chapter (multiple visible chapters all highlight) - replace the accent fill with a subtle translucent background contrast lift; no spied-section text recoloring - add an elastic water-droplet animation on range entry, reduced-motion gated
- replace per-row backgrounds with a single measured overlay band so adjacent chapters render seamlessly with no visible boundary lines - swap the flat per-row scale bounce for an elastic top/height transition, so the highlight grows vertically out of the previous range into the next - reduced-motion keeps only the fade
- when the spied range reaches a section outside the TOC's bounded scroll viewport, scroll the TOC just enough to keep it in view and follow the reader - minimal directional scroll with reduced-motion honored (instant vs smooth) - runs only on range changes so manual TOC browsing is not interrupted
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
review @codex |
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
feat: add vim-like memory browse keybindings
|
review @codex @copilot @coderabbitai |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/workflows/task-22-vim-like-key-bindings.md (1)
9-15: ⚡ Quick winAdd
docs/INDEX.mdto the required context list.This workflow points readers at supporting docs, but it skips the repo’s required orientation entry point. Adding it here keeps the workflow aligned with the documentation navigation contract. As per coding guidelines,
docs/**/*: All project detail lives underdocs/directory. Start at docs/INDEX.md for orientation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/workflows/task-22-vim-like-key-bindings.md` around lines 9 - 15, In the "## Required Context" section of task-22-vim-like-key-bindings.md add an entry that links to the repository orientation file docs/INDEX.md so the workflow includes the docs entry point; update the list by inserting a bullet like "- [Project orientation](../docs/INDEX.md)" (or similar phrasing) immediately among the existing bullets so the Required Context block references docs/INDEX.md.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/workflows/task-22-vim-like-key-bindings.md`:
- Around line 65-69: The step in the MemoryBrowse implementation refers to the
outdated symbol filteredMemories; update the text to reference the current
browse list name visibleMemories instead (e.g., "Derive selected index from
visibleMemories") and ensure any related phrasing about clamping movement or
preserving selection mentions MemoryBrowse and visibleMemories so it matches the
actual code symbols used.
---
Nitpick comments:
In `@docs/workflows/task-22-vim-like-key-bindings.md`:
- Around line 9-15: In the "## Required Context" section of
task-22-vim-like-key-bindings.md add an entry that links to the repository
orientation file docs/INDEX.md so the workflow includes the docs entry point;
update the list by inserting a bullet like "- [Project
orientation](../docs/INDEX.md)" (or similar phrasing) immediately among the
existing bullets so the Required Context block references docs/INDEX.md.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6cbc33e6-723b-4967-857f-161dd3e1a488
📒 Files selected for processing (13)
docs/references/design-system/reader-and-content.mddocs/workflows/README.mddocs/workflows/task-22-vim-like-key-bindings.mde2e/browse-shell.spec.tssrc/components/memories/MemoryBrowse.tsxsrc/components/memories/MemorySearchBar.tsxsrc/components/reader/MemoryReader.tsxsrc/components/reader/toc-reading-range.tssrc/components/reader/toc-scroll-spy.tssrc/styles/tailwind.csstests/components/memory-browse-actions.test.tstests/components/reader-toc-reading-range.test.tstests/components/reader-toc-scroll-spy.test.ts
✅ Files skipped from review due to trivial changes (1)
- docs/workflows/README.md
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/components/reader-toc-reading-range.test.ts
- src/components/reader/toc-scroll-spy.ts
- src/styles/tailwind.css
- tests/components/reader-toc-scroll-spy.test.ts
- src/components/reader/MemoryReader.tsx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e27a0ca47
ℹ️ 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".
|
Handled the new review feedback on PR #25 and pushed the follow-up in signed commits 65c4d9d and 9b8fc75. Summary:
Validation:
@coderabbitai please review again. Codex reviewer, please re-review the latest pushed commit. |
|
Triggering a fresh incremental review of the two new commits now. ✅ Actions performedReview triggered.
|
|
review @codex |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Summary
Verification
git diff --checkenv TMPDIR=/private/tmp BUN_INSTALL_CACHE_DIR=/private/tmp/bun-cache /Users/vvx/.local/share/mise/installs/bun/1.3.13/bin/bun /Users/vvx/projekt/www/trauma/node_modules/vitest/vitest.mjs run tests/server/browse-limits.test.ts tests/components/memory-browse-actions.test.ts tests/server/flashbacks/repository.test.ts-> 27 tests passedenv TMPDIR=/private/tmp BUN_INSTALL_CACHE_DIR=/private/tmp/bun-cache /Users/vvx/.local/share/mise/installs/bun/1.3.13/bin/bun /Users/vvx/projekt/www/trauma/node_modules/vitest/vitest.mjs run tests/server/memories/browse.test.ts tests/server/reader/page-data.test.ts tests/components/reader-flashback-tabs.test.ts tests/components/browse-loader.test.ts tests/components/flashbacks-loader.test.ts tests/components/memory-browse-actions.test.ts tests/server/flashbacks/repository.test.ts tests/server/browse-limits.test.ts-> 60 tests passed/Users/vvx/.local/share/mise/installs/bun/1.3.13/bin/bun run verify-> typecheck, 104 test files / 805 passed / 5 todo, and build passed after rerunning outside sandbox for Unix socket and GPG fixture-git testsNotes
.tmp/output was left uncommitted.Summary by CodeRabbit
New Features
UI
Documentation
Tests
Database