feat: show path and state metadata in the managed attachments comment#370
Conversation
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
uploads-auth | 2cb16d3 | Commit Preview URL Branch Preview URL |
Jul 22 2026, 12:13 AM |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
uploads-api | 2cb16d3 | Commit Preview URL Branch Preview URL |
Jul 22 2026, 12:12 AM |
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (2)
🚫 Excluded labels (none allowed) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@apps/api/src/github-comment-render.ts`:
- Around line 138-141: Update escapeMarkdownText in
apps/api/src/github-comment-render.ts at lines 138-141 and the corresponding
renderer helper in packages/uploads/src/github.ts at lines 248-251 to include ~
in the escaped markdown character class. Add matching coverage for literal tilde
metadata values in both renderer test suites.
In `@packages/uploads/src/commands.ts`:
- Around line 684-700: Update the listAll call in the attachment item mapping to
request only the canonical metadata keys path and state through its
metadata-filtering option, and assert that filtered request in the associated
test path. Keep the existing mapping and rendering behavior unchanged.
🪄 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: CHILL
Plan: Pro
Run ID: 4fb648c8-8334-4074-ab5c-56634aa7c680
📒 Files selected for processing (20)
.changeset/attachment-comment-metadata.mdapps/api/src/file-metadata.tsapps/api/src/github-comment-render.test.tsapps/api/src/github-comment-render.tsapps/api/src/github-comment.test.tsapps/api/src/github-comment.tsapps/api/src/routes/admin-ui.test.tsapps/api/src/routes/admin-ui.tsapps/api/src/routes/files.tsapps/api/src/workspace.tsapps/api/test/file-metadata-sqlite.test.tsapps/api/test/helpers/fake-file-metadata-table.tsapps/api/test/routes-files.test.tspackages/uploads/src/client.tspackages/uploads/src/commands.tspackages/uploads/src/github.tspackages/uploads/test/client-metadata.test.tspackages/uploads/test/commands-comment.test.tspackages/uploads/test/github-render-golden.test.tstest/fixtures/github-comment-golden-meta.json
| /** Backslash-escape the markdown metacharacters that can appear in a metadata value. */ | ||
| function escapeMarkdownText(s: string): string { | ||
| return s.replace(/([\\`*_[\]])/g, "\\$1"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Escape ~ in literal markdown captions.
GitHub interprets ~~value~~ as strikethrough, so a valid printable metadata value is not rendered verbatim in list and overflow captions. Include ~ in the escaped character class and add the same coverage in both renderer test suites.
apps/api/src/github-comment-render.ts#L138-L141: escape~inescapeMarkdownText.packages/uploads/src/github.ts#L248-L251: apply the identical escape change.
📍 Affects 2 files
apps/api/src/github-comment-render.ts#L138-L141(this comment)packages/uploads/src/github.ts#L248-L251
🤖 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 `@apps/api/src/github-comment-render.ts` around lines 138 - 141, Update
escapeMarkdownText in apps/api/src/github-comment-render.ts at lines 138-141 and
the corresponding renderer helper in packages/uploads/src/github.ts at lines
248-251 to include ~ in the escaped markdown character class. Add matching
coverage for literal tilde metadata values in both renderer test suites.
| const items: AttachmentItem[] = ( | ||
| await client.listAll({ prefix: ghKeyPrefix(target), metadata: true }) | ||
| ).map(({ key, url, embedUrl, pageUrl, metadata }) => { | ||
| // The list endpoint returns every metadata key; the comment renders only | ||
| // these two. Narrowing here keeps both render paths byte-identical. | ||
| const path = metadata?.path; | ||
| const state = metadata?.state; | ||
| return { | ||
| key, | ||
| url, | ||
| embedUrl, | ||
| pageUrl, | ||
| ...(path || state | ||
| ? { meta: { ...(path ? { path } : {}), ...(state ? { state } : {}) } } | ||
| : {}), | ||
| }; | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Request only canonical metadata keys in the CLI fallback.
Line 685 fetches every metadata field, including fields such as device and software, before dropping them locally. This violates the requirement to enforce metadata privacy at query time. Extend the listing contract to request only path and state, and assert that filtered request in this test path.
🤖 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 `@packages/uploads/src/commands.ts` around lines 684 - 700, Update the listAll
call in the attachment item mapping to request only the canonical metadata keys
path and state through its metadata-filtering option, and assert that filtered
request in the associated test path. Keep the existing mapping and rendering
behavior unchanged.
GitHub's strikethrough extension treats a matching pair of one or two tildes as markup, so a metadata value like /a~b~c rendered with b struck through in the markdown list-suffix context. Same class as the * and _ already escaped. Both renderer copies and both test suites.
📎 Attachments
Maintained by uploads.sh — re-uploading a file with the same name updates it everywhere it is embedded. |
1 participant
Closes #365.
In plain terms
A screenshot attached to a PR used to show up as
settings-a3f9.webp. If it was uploaded with--state beforeon/settings, it now shows up as/settings · beforeunderneath the image. Attachments without that metadata render exactly as they did before.#361 added the canonical metadata vocabulary; the managed comment ignored all of it. This connects the two.
What it renders
Inline images get a
<sub>caption; list rows and the collapsed overflow list get a·suffix after the filename.The third shot has no metadata and renders byte-for-byte as it does on
maintoday.The cost question #365 raised
The issue flagged that this adds a D1 read to a path that runs on every webhook delivery, and asked for a rows-read estimate against #352's perf work before shipping. Measured against production D1 rather than estimated — running the real statement shape and reading
rows_readoff the response:rows_readgetMetadataForKeys)AND meta_key IN ('path','state')So
unfiltered ≈ 2·K + total_rows,filtered ≈ 3·K + matched. Two things follow:(workspace, object_key, meta_key)primary-key index. Production attachment counts are 1–2 per PR at the median, 8 at the max, so a median delivery pays ~5–10 rows.autoPromoteAndCommentrunspromoteBranchAttachmentsbeforegatherAndUpsert, and that already does a full unfilteredgetMetadataForKeysover the staged keys on everyopened/reopened/synchronize.gatherGalleriesis two more D1 queries. "No D1 on this path" was true ofgatherAttachmentsin isolation, not of the sync path.No perf redesign needed — just a key filter on the existing primitive.
Privacy
The comment posts publicly, on repos whose visibility uploads.sh does not check.
deviceandsoftwarecome from EXIF, so they must not end up there. The restriction lives at the query, not the renderer: the server never fetches them for this path, so a later renderer bug can't leak what was never read.Reinforced twice more: the CLI's fallback narrows at its mapping site, and
AttachmentItem.metais typed as{ path?: string; state?: string }rather than a metadata bag, so the narrow set is a compile-time property.Degradation
The requirement was that a comment with no metadata look exactly like it does today.
test/fixtures/github-comment-golden.jsonandgithub-comment-golden-cap.jsonwere deliberately not regenerated — their items carry nometa, so correct code has to reproduce them byte-for-byte. Leaving them untouched turns them into the regression test. A new third fixture covers the metadata cases from both render paths.Decisions worth flagging
pathandstateonly.viewport/device/softwareare noise in a PR context;urlwraps badly. The privacy argument independently points at the same narrow set.githubCommentShowMetadata, default-on, mirroringgithubCommentLinkToFilePage(Make managed-comment file-page linking configurable (per-workspace setting / in-repo config) #304) exactly — KV-backed, admin-API only, no web UI. Not a configurable key list.ghfallback gets the same data via a new opt-in?metadata=1onGET /v1/:workspace/files, so the two render paths don't diverge and the golden fixture stays a real cross-check. It inherits Make managed-comment file-page linking configurable (per-workspace setting / in-repo config) #304's known caveat: the CLI has noWorkspaceRecordin scope, so a workspace that sets the flagfalseand falls through toghstill gets captions.before/afterpairs side by side is the obvious next win and is deliberately out of scope.Not in scope
Re-rendering historical comments. New renders only.
Test plan
pnpm test— 173 files / 2194 testspnpm typecheck,pnpm lintclean--stateRollout
No D1 migration — the flag is KV, and the filter uses the existing index. Deploy order doesn't matter:
metadata=1is additive, and an older worker ignoring it degrades the CLI fallback to today's output.Summary by CodeRabbit
New Features
Bug Fixes