Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughThis PR introduces a complete "Reader mode" feature for viewing archived memories. It adds server-side Markdown rendering with HTML sanitization, table-of-contents generation, a new route to display individual memories, UI components with responsive styling, and comprehensive tests including end-to-end verification. ChangesMemory Reader Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 67490eb2a1
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96eecb98a6
ℹ️ 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: 3
🧹 Nitpick comments (5)
tests/server/reader/route-state.test.ts (1)
36-56: ⚡ Quick winConsider adding a test case for the ready state.
The test suite covers error states returning HTTP status codes, but doesn't verify that a "ready" result returns
undefined(no error status). While not critical, it would complete the coverage.📝 Suggested additional test case
it("maps reader error results to HTTP status codes", () => { expect(readerHttpStatusCode(undefined)).toBeUndefined(); + expect( + readerHttpStatusCode({ + status: "ready", + memory: { /* minimal ready result */ }, + content: { relativePath: "..." }, + rendered: { html: "", toc: [] }, + }), + ).toBeUndefined(); expect( readerHttpStatusCode({🤖 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/server/reader/route-state.test.ts` around lines 36 - 56, Add a test asserting that a successful/ready reader result maps to no HTTP error: call readerHttpStatusCode with a result object whose status is "ready" (include a representative message or metadata) and expect the return to be undefined; update the test in route-state.test.ts alongside the existing cases so readerHttpStatusCode({ status: "ready", message: "Ready for reading." }) is asserted toBeUndefined().e2e/reader.spec.ts (2)
24-145: ⚡ Quick winConsider extracting the shared Bun-resolution helpers.
resolveBunExecutable,isBunExecutable, andcanAccessare duplicated between this file andtests/server/reader/page-data.test.ts. A small shared helper module (e.g.,tests/_helpers/bun-executable.ts) would prevent the two copies from drifting (the constants/paths already differ slightly between them).🤖 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 `@e2e/reader.spec.ts` around lines 24 - 145, Extract the duplicated Bun resolver helpers into a single shared module and update both specs to import them: move resolveBunExecutable, isBunExecutable, and canAccess (and any candidate paths/constants like the bun install path or BUN_EXECUTABLE handling) into a new tests/_helpers/bun-executable.ts, export those functions, and replace the local implementations in e2e/reader.spec.ts and tests/server/reader/page-data.test.ts with imports from that helper so both files use the same logic.
122-145: 💤 Low valueDead code in
resolveBunExecutablecandidates list (mirrorstests/server/reader/page-data.test.ts).Same issue: line 129 (
process.versions.bun !== undefined ? process.execPath : undefined) is unreachable because of the early return at lines 123-125.♻️ Proposed cleanup
const candidates = [ process.env.BUN_EXECUTABLE, - process.versions.bun !== undefined ? process.execPath : undefined, `${homedir()}/.local/share/mise/installs/bun/1.3.13/bin/bun`, process.env.npm_execpath, "bun", ];🤖 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 `@e2e/reader.spec.ts` around lines 122 - 145, The candidates array in resolveBunExecutable contains a redundant entry "process.versions.bun !== undefined ? process.execPath : undefined" that is unreachable because resolveBunExecutable already returns early when process.versions.bun is defined; remove that ternary element from the candidates list so the array only contains viable alternatives (e.g., process.env.BUN_EXECUTABLE, the homedir path, process.env.npm_execpath, and "bun"), keeping the early-return logic in resolveBunExecutable unchanged.tests/server/reader/page-data.test.ts (1)
190-213: 💤 Low valueDead code in
resolveBunExecutablecandidates list.Line 197 (
process.versions.bun !== undefined ? process.execPath : undefined) is unreachable: the early return at lines 191-193 already covers that condition, so by the time we buildcandidateswe knowprocess.versions.bun === undefined, making this entry alwaysundefined.♻️ Proposed cleanup
const candidates = [ process.env.BUN_EXECUTABLE, - process.versions.bun !== undefined ? process.execPath : undefined, join(homedir(), ".local/share/mise/installs/bun/1.3.13/bin/bun"), process.env.npm_execpath, "bun", ];🤖 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/server/reader/page-data.test.ts` around lines 190 - 213, The candidates array in resolveBunExecutable contains a redundant entry "process.versions.bun !== undefined ? process.execPath : undefined" which is unreachable because the function already returns when process.versions.bun !== undefined; remove that conditional entry from the candidates list (keep other candidates like process.env.BUN_EXECUTABLE, the homedir path, process.env.npm_execpath, and "bun") and ensure the rest of the logic (find, isBunExecutable, canAccess, and the final throw) remains unchanged.src/server/reader/markdown-renderer.ts (1)
292-307: 💤 Low valueOptional: tighten the Vimeo embed path check.
For
www.youtube.com/www.youtube-nocookie.comthe pathname is required to start with/embed/, butplayer.vimeo.comis allowed on any path. Sinceplayer.vimeo.comis dedicated to embeds in practice, this is acceptable, but addingurl.pathname.startsWith("/video/") || url.pathname.startsWith("/event/")for Vimeo would mirror the YouTube tightness and reject anomalous paths.🤖 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/reader/markdown-renderer.ts` around lines 292 - 307, The isAllowedIframeSource function currently allows player.vimeo.com on any path; tighten it by requiring Vimeo embed paths to match expected prefixes: change the return condition in isAllowedIframeSource (referencing ALLOWED_IFRAME_HOSTNAMES and url.hostname) so that for www.youtube.com and www.youtube-nocookie.com you still require url.pathname.startsWith("/embed/"), and for player.vimeo.com require the pathname to start with one of the valid Vimeo embed prefixes (e.g. url.pathname.startsWith("/embed/") || url.pathname.startsWith("/video/") || url.pathname.startsWith("/event/")), returning false otherwise.
🤖 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 `@app.config.ts`:
- Around line 3-18: The config uses an invalid Solid Start option
server.externals.traceInclude; replace it with the appropriate supported option
depending on intent: if you want Rollup to treat drizzle-orm/bun-sqlite as
external (prevent bundling), move the array into server.rollupConfig.external;
if you want Nitro to include/traces those deps during build, move the array into
server.traceDeps; update the property name(s) and keep the same string patterns
(originally listed under server.externals.traceInclude) so the
drizzle-orm/bun-sqlite entries are recognized by Solid Start.
In `@src/server/reader/page-data.ts`:
- Around line 72-98: The catch is too broad because the `error instanceof Error`
branch swallows every unexpected failure; update the catch to only handle known
types (`MemoryContentStoreError` with `code === "missing_content"` and
`TraumaConfigError` and other specific MemoryContentStoreError cases) and remove
the `error instanceof Error` arm so unknown errors propagate; for unexpected
values, log the full error (e.g., via your logger or `console.error`) including
message and stack before rethrowing to preserve diagnostics; keep the existing
`finally { connection?.close(); }` behavior.
In `@src/styles/app.css`:
- Around line 293-296: The font-family declaration in the .reader-content code
selector uses quoted SFMono-Regular which violates the font-family-name-quotes
rule; update the font-family value for the .reader-content code rule to remove
the quotes around SFMono-Regular (leave other family names and the monospace
fallback unchanged) so it becomes SFMono-Regular, Consolas, "Liberation Mono",
monospace.
---
Nitpick comments:
In `@e2e/reader.spec.ts`:
- Around line 24-145: Extract the duplicated Bun resolver helpers into a single
shared module and update both specs to import them: move resolveBunExecutable,
isBunExecutable, and canAccess (and any candidate paths/constants like the bun
install path or BUN_EXECUTABLE handling) into a new
tests/_helpers/bun-executable.ts, export those functions, and replace the local
implementations in e2e/reader.spec.ts and tests/server/reader/page-data.test.ts
with imports from that helper so both files use the same logic.
- Around line 122-145: The candidates array in resolveBunExecutable contains a
redundant entry "process.versions.bun !== undefined ? process.execPath :
undefined" that is unreachable because resolveBunExecutable already returns
early when process.versions.bun is defined; remove that ternary element from the
candidates list so the array only contains viable alternatives (e.g.,
process.env.BUN_EXECUTABLE, the homedir path, process.env.npm_execpath, and
"bun"), keeping the early-return logic in resolveBunExecutable unchanged.
In `@src/server/reader/markdown-renderer.ts`:
- Around line 292-307: The isAllowedIframeSource function currently allows
player.vimeo.com on any path; tighten it by requiring Vimeo embed paths to match
expected prefixes: change the return condition in isAllowedIframeSource
(referencing ALLOWED_IFRAME_HOSTNAMES and url.hostname) so that for
www.youtube.com and www.youtube-nocookie.com you still require
url.pathname.startsWith("/embed/"), and for player.vimeo.com require the
pathname to start with one of the valid Vimeo embed prefixes (e.g.
url.pathname.startsWith("/embed/") || url.pathname.startsWith("/video/") ||
url.pathname.startsWith("/event/")), returning false otherwise.
In `@tests/server/reader/page-data.test.ts`:
- Around line 190-213: The candidates array in resolveBunExecutable contains a
redundant entry "process.versions.bun !== undefined ? process.execPath :
undefined" which is unreachable because the function already returns when
process.versions.bun !== undefined; remove that conditional entry from the
candidates list (keep other candidates like process.env.BUN_EXECUTABLE, the
homedir path, process.env.npm_execpath, and "bun") and ensure the rest of the
logic (find, isBunExecutable, canAccess, and the final throw) remains unchanged.
In `@tests/server/reader/route-state.test.ts`:
- Around line 36-56: Add a test asserting that a successful/ready reader result
maps to no HTTP error: call readerHttpStatusCode with a result object whose
status is "ready" (include a representative message or metadata) and expect the
return to be undefined; update the test in route-state.test.ts alongside the
existing cases so readerHttpStatusCode({ status: "ready", message: "Ready for
reading." }) is asserted toBeUndefined().
🪄 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: 5d3ae12c-72de-42ad-aad3-31f541c1b6a2
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
app.config.tse2e/reader.spec.tspackage.jsonplaywright.config.tssrc/components/reader/MemoryReader.tsxsrc/components/reader/source-url.tssrc/routes/memories/[id].tsxsrc/routes/memories/reader-route-state.tssrc/server/db/connection.tssrc/server/reader/markdown-renderer.tssrc/server/reader/page-data.tssrc/styles/app.csstests/server/reader/markdown-renderer.test.tstests/server/reader/page-data.test.tstests/server/reader/route-state.test.tstests/server/reader/source-url.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb35791886
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 178ffde0d2
ℹ️ 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".
nonnil
left a comment
nonnil
left a comment
There was a problem hiding this comment.
There are issues, but since we’re getting nowhere, we’ll handle them all together in a different task.
1 participant
Summary
Implements the
/memories/:idreader workflow for TRA-7:CONTENT.mdthrough the existing repository/store contracts.Markdown libraries chosen
markdown-itfor Markdown/GFM-style tables, strikethrough, lists, links, and code fences.markdown-it-footnotefor footnote syntax.markdown-it-anchorfor deterministic heading IDs.highlight.jsfor fenced-code syntax highlighting.sanitize-htmlfor the final HTML allowlist.Sanitization allowlist
The renderer allows common Markdown output tags, heading
idattributes, safe link/image attributes, code highlighting classes, footnote classes, and persisted highlight markers viamark[data-highlight-id].Controlled embeds are limited to HTTPS iframe sources from YouTube/YouTube NoCookie and Vimeo embed hosts. Scripts, event handlers,
javascript:URLs, and non-allowlisted iframe sources are removed.E2E fixture
The Playwright reader smoke uses fixture memory
018f04a2-3c6f-7c88-9a8b-8c99a9b7f101and writes its ignored config/store/database under.trauma/e2ebefore visiting/memories/:id.Validation
TMPDIR=$PWD/.tmp/bun-tmp BUN_INSTALL_CACHE_DIR=$PWD/.tmp/bun-cache MISE_TRUSTED_CONFIG_PATHS=$PWD/mise.toml mise exec -C . -- bun run typecheck- passed.TMPDIR=$PWD/.tmp/bun-tmp BUN_INSTALL_CACHE_DIR=$PWD/.tmp/bun-cache MISE_TRUSTED_CONFIG_PATHS=$PWD/mise.toml mise exec -C . -- bun run test- passed, 6 files / 31 tests.TMPDIR=$PWD/.tmp/bun-tmp BUN_INSTALL_CACHE_DIR=$PWD/.tmp/bun-cache MISE_TRUSTED_CONFIG_PATHS=$PWD/mise.toml mise exec -C . -- bun run build- passed.git diff --check- passed.TMPDIR=$PWD/.tmp/bun-tmp BUN_INSTALL_CACHE_DIR=$PWD/.tmp/bun-cache MISE_TRUSTED_CONFIG_PATHS=$PWD/mise.toml mise exec -C . -- bun run test:e2e- blocked by sandbox before page load: Chromium fails to launch withbootstrap_check_in org.chromium.Chromium.MachPortRendezvousServer... Permission denied (1100).Pre-push also reran
typecheck,test, andbuildsuccessfully.Summary by CodeRabbit
Release Notes
New Features
Tests