Add Desktop and Web artifact workflows - #181
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR adds session-scoped Artifacts for Web and Desktop. It adds explicit registration, durable metadata, secure file access, real-time updates, previews, native actions, encrypted Cloud sharing, UI integration, and prototype validation. ChangesArtifact delivery
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Agent as show_artifact
participant Registry as artifact.Service
participant Session as session.Recorder
participant Web as WebSocket bridge
participant UI as ArtifactsPanel
participant Cloud as Cloud share API
Agent->>Registry: Register workspace file
Registry->>Session: Append artifact metadata
Agent->>Web: Emit artifact_upserted
Web->>UI: Refresh task and artifact state
UI->>Registry: Request validated content
UI->>Cloud: Create encrypted share
Cloud-->>UI: Return fragment-key share URL
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
cnjack
marked this pull request as ready for review
August 2, 2026 02:30
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (8)
web/src/components/ArtifactsPanel.tsx (2)
139-139: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThe fullscreen overlay mounts a second
ArtifactViewerfor the same artifact.
viewerat Line 139 is created unconditionally and rendered at Line 214. Whenfullscreenis true, the inline panel stays mounted behind the fixed overlay, and Line 226 mounts a secondArtifactViewerwith the samerecord. Each instance runs its ownapi.artifactContenteffect, so the artifact is fetched twice and two object URLs are held. Skip the inline viewer while the overlay is open.♻️ Proposed change
- <div className="min-h-0 flex-1 overflow-auto p-3">{viewer}</div> + <div className="min-h-0 flex-1 overflow-auto p-3">{fullscreen ? null : viewer}</div>Also applies to: 214-214, 226-226
🤖 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 `@web/src/components/ArtifactsPanel.tsx` at line 139, Update the viewer rendering in ArtifactsPanel so the inline ArtifactViewer created by the viewer expression is not mounted while fullscreen is true. Preserve the fullscreen overlay’s single ArtifactViewer instance and continue rendering the inline viewer when fullscreen is false.
327-331: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the active-share filter.
The expression
shares.filter((share) => !share.revoked_at && share.state !== 'revoked')appears at Line 327 and Line 331. The two copies must stay in sync, and the list is computed twice per render. Hoist it into auseMemoand use the result in both places.🤖 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 `@web/src/components/ArtifactsPanel.tsx` around lines 327 - 331, In ArtifactsPanel, extract the duplicated active-share predicate into a useMemo-backed value near the component’s other derived state, using shares as its dependency. Replace both occurrences of the inline shares.filter expression in the active-share section and map with that memoized result.internal/session/artifact_test.go (1)
43-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the workspace absolute path is absent.
file_contentandabsolute_pathare key names thatEntrynever marshals, so these two checks always pass. The real privacy risk is the absolute workspace path. Addworkspaceto the forbidden list to make the assertion meaningful.♻️ Proposed change
- for _, forbidden := range []string{"<html>", "file_content", "absolute_path"} { + for _, forbidden := range []string{"<html>", "file_content", "absolute_path", workspace} { if strings.Contains(string(raw), forbidden) { t.Fatalf("session entry leaked %q: %s", forbidden, raw) } }🤖 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 `@internal/session/artifact_test.go` around lines 43 - 47, Update the forbidden-token list in the session artifact test to include “workspace”, so the marshaled entry is explicitly checked for absence of the workspace absolute path. Keep the existing “<html>”, “file_content”, and “absolute_path” checks unchanged.internal/web/models_test.go (1)
30-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that
disabledModelIDexists in the catalog response.
addedByIDis amap[string]bool. If the generated catalog stops shippingk3,addedByID["k3"]returns the zero valuefalse, and the check at Line 93 passes without testing anything. The enabled-model check is protected by theLookupModelassertion at Line 64, but the disabled model has no equivalent guard. Assert presence before asserting the value.♻️ Proposed change
- addedByID := make(map[string]bool, len(got)) + addedByID := make(map[string]bool, len(got)) for _, item := range got { addedByID[item.ID] = item.Added } + if _, present := addedByID[disabledModelID]; !present { + t.Fatalf("catalog did not return %s; fixture is stale", disabledModelID) + } if !addedByID[enabledModelID] {Also applies to: 90-95
🤖 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 `@internal/web/models_test.go` around lines 30 - 34, Strengthen the catalog test around disabledModelID by first asserting that disabledModelID exists in the catalog response, then asserting its recorded value is false. Update the check using addedByID so a missing k3 entry cannot pass through its map zero value.web/src/app/wsBridge.artifact.test.ts (1)
18-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the
focus: falseandd.idfallback branches.
onArtifactUpsertedinweb/src/app/wsBridge.tshas three guards: task match,d.focus !== false, andd.artifact_id || d.id. The tests cover only the task-match guard. Add two cases: an active-task event withfocus: falsemust not emit, and an active-task event carryingidinstead ofartifact_idmust emit withartifact_idnormalized.🤖 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 `@web/src/app/wsBridge.artifact.test.ts` around lines 18 - 44, Extend the artifact WebSocket bridge tests around onArtifactUpserted with an active-task event using focus: false and assert that it emits nothing, then add an active-task event providing id without artifact_id and assert it emits with artifact_id normalized to that id. Preserve the existing task-matching and background-artifact coverage.web/src/components/AutomationsView.tsx (1)
877-883: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGive the artifact badge an accessible name.
The badge renders an icon and a bare number. A screen reader announces only the digit, next to the trigger-kind chip. The unseen dot carries meaning but has no text. Add a
titleto the badge and mark the dotaria-hidden. Reuse the existingrightPanel.artifactskey so no new translation entry is needed.♻️ Proposed change
{!!r.artifact_count && ( - <span className="inline-flex items-center gap-1 rounded-[var(--radius-sm)] bg-[var(--color-muted)] px-1.5 py-px text-[10.5px] font-semibold text-[var(--color-muted-foreground)]"> - <DocumentDuplicateIcon className="h-3 w-3" /> + <span + title={`${t('rightPanel.artifacts')} · ${r.artifact_count}`} + className="inline-flex items-center gap-1 rounded-[var(--radius-sm)] bg-[var(--color-muted)] px-1.5 py-px text-[10.5px] font-semibold text-[var(--color-muted-foreground)]" + > + <DocumentDuplicateIcon aria-hidden className="h-3 w-3" /> {r.artifact_count} - {r.artifact_unseen && <span className="h-1.5 w-1.5 rounded-full bg-[var(--color-accent-neutral)]" />} + {r.artifact_unseen && <span aria-hidden className="h-1.5 w-1.5 rounded-full bg-[var(--color-accent-neutral)]" />} </span> )}🤖 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 `@web/src/components/AutomationsView.tsx` around lines 877 - 883, Update the artifact badge in the AutomationsView render block to add a title using the existing rightPanel.artifacts translation key, and mark the unseen indicator dot aria-hidden so it is ignored by screen readers while preserving its visual meaning.internal/cloud/artifact_share.go (1)
134-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the cleanup revoke failure.
The deferred cleanup discards the revoke error. If the revoke fails, an incomplete share intent stays on the server with no local diagnostic. Send the failure through
config.Logger().♻️ Proposed change
cleanupCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 10*time.Second) defer cancel() - _ = p.revoke(cleanupCtx, client, token, intent.ShareID) + if revokeErr := p.revoke(cleanupCtx, client, token, intent.ShareID); revokeErr != nil { + config.Logger().Printf("[artifact-share] revoke incomplete intent %s: %v", intent.ShareID, revokeErr) + } }()As per coding guidelines: "Send all diagnostics through
config.Logger()" and "The cloud connector must ... log failures".🤖 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 `@internal/cloud/artifact_share.go` around lines 134 - 142, Update the deferred cleanup in the share operation to capture the error returned by p.revoke and log any failure through config.Logger(). Preserve the existing timeout context, cancellation, and completed guard, while ensuring successful revocations remain silent.Source: Coding guidelines
web/src/lib/api.ts (1)
40-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared auth and error handling.
requestBlobandrequestVoidrepeat the same four steps: build headers, attach the bearer token, callnotifyAuthExpiredon 401, and parse a JSON error body. The existingrequesthelper performs the same steps. Extract one helper that returns the rawResponse, then let each caller decode it.♻️ Proposed change
+async function rawRequest(path: string, init?: RequestInit): Promise<Response> { + const headers = new Headers(init?.headers) + const token = getAuthToken() + if (token) headers.set('Authorization', `Bearer ${token}`) + const resp = await fetch(`${apiBase}${path}`, { ...init, headers }) + if (resp.status === 401) notifyAuthExpired() + if (!resp.ok) { + const body = await resp.json().catch(() => ({ error: resp.statusText })) + throw new Error(body.error || `HTTP ${resp.status}`) + } + return resp +} + async function requestBlob(path: string): Promise<Blob> { - const headers = new Headers() - const token = getAuthToken() - if (token) headers.set('Authorization', `Bearer ${token}`) - const resp = await fetch(`${apiBase}${path}`, { headers, cache: 'no-store' }) - if (resp.status === 401) notifyAuthExpired() - if (!resp.ok) { - const body = await resp.json().catch(() => ({ error: resp.statusText })) - throw new Error(body.error || `HTTP ${resp.status}`) - } - return resp.blob() + const resp = await rawRequest(path, { cache: 'no-store' }) + return resp.blob() } async function requestVoid(path: string, method: string): Promise<void> { - const headers = new Headers() - const token = getAuthToken() - if (token) headers.set('Authorization', `Bearer ${token}`) - const resp = await fetch(`${apiBase}${path}`, { method, headers }) - if (resp.status === 401) notifyAuthExpired() - if (!resp.ok) { - const body = await resp.json().catch(() => ({ error: resp.statusText })) - throw new Error(body.error || `HTTP ${resp.status}`) - } + await rawRequest(path, { method }) }🤖 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 `@web/src/lib/api.ts` around lines 40 - 63, Refactor requestBlob and requestVoid to reuse a shared raw-Response helper, preferably the existing request helper, for header construction, bearer-token attachment, fetch execution, 401 notification, and JSON error handling. Keep requestBlob responsible for decoding the successful response as a Blob and requestVoid for completing without a body, while removing their duplicated auth and error-handling logic.
🤖 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 `@internal-doc/artifacts-prd.md`:
- Line 237: Update the “安全预览的最低合同” specification to require a server-generated
CSP that denies network access by default, using explicit local-only sources for
any required scripts, styles, images, and fonts. Add negative tests covering
remote script, image, stylesheet, fetch, and form-submission attempts, while
preserving the existing iframe sandbox and sensitive-path restrictions.
In `@internal-doc/artifacts-ui/index.html`:
- Around line 966-981: Update the ⇧⌘A shortcut in the document keydown handler
to clear the inline app.style.gridTemplateColumns value before or alongside
setVariant('docked'), allowing the docked CSS layout to restore the Artifacts
panel. Preserve the existing preventDefault and variant behavior.
In `@internal/artifact/service.go`:
- Around line 418-445: Update classifyFile so the detected kind from
kindForExtension remains authoritative and a non-KindAuto hint cannot replace
it. Treat hint only as optional narrowing/validation if supported by the
existing contract; otherwise ignore it while preserving validation of supported
values and leaving media-type detection unchanged.
In `@internal/cloud/artifact_share_test.go`:
- Around line 87-102: Replace the t.Fatalf authorization assertions in the
intent and upload HTTP handlers with t.Errorf, then return an appropriate error
response immediately when authorization is invalid. Keep the valid-authorization
behavior unchanged so handlers always produce a complete response before
returning.
- Around line 189-212: Protect the shared revoked state in
TestArtifactSharePublisherRevokesIntentWhenUploadFails with a sync.Mutex,
matching the sibling test: lock around the DELETE handler’s write and around the
final assertion’s read, and add the required sync import if absent.
In `@internal/cloud/artifact_share.go`:
- Around line 297-304: Update artifactShareURL to accept https URLs universally
but reject non-loopback http URLs; permit http only when the parsed host is a
loopback address, while preserving the existing validation for missing hosts,
credentials, and fragments.
In `@internal/tools/artifact.go`:
- Line 33: Change Env.NewShowArtifactTool to take no arguments and return
tool.InvokableTool, retrieving ShowArtifactDeps from the session-scoped Env
instead. Update its callers to store the dependencies on Env before constructing
the tool, preserving consistent recreation during mode transitions.
In `@internal/web/artifacts_desktop.go`:
- Around line 53-60: Update blockedArtifactHostOpenExtensions to include the
missing Windows active-content extensions .scf, .chm, .mht, and .mhtml, ensuring
handleOpenArtifact routes these artifacts through the sandboxed viewer.
In `@internal/web/artifacts.go`:
- Around line 218-290: Update decodeArtifactShareRequest to parse and return the
documented Revision field alongside the expiry duration, preserving strict JSON
validation. In handleCreateArtifactShare, compare the requested revision with
record.Revision after opening the artifact and return a conflict response with
error "artifact_revision_conflict" when they differ; only snapshot and publish
the artifact when the revisions match.
- Around line 28-44: Update artifactWorkspace to reject remote workspaces even
when resolveEngine(sessionID) returns nil, including inactive or automation
sessions. Use the session/project metadata available to determine whether the
workspace is remote before accepting the workspace returned by
workspacePwdForTask, while preserving existing validation and missing-workspace
behavior.
- Around line 76-82: Update setArtifactContentHeaders to select the
Content-Security-Policy based on record.Kind: use the documented HTML policy for
KindHTML, including img-src data: blob:, style-src 'unsafe-inline', and
script-src 'unsafe-inline'; retain the existing strict policy for all other
artifact kinds.
In `@web/src/App.tsx`:
- Around line 417-423: Update the unseen-artifact indicator inside the artifacts
button in App.tsx to expose an accessible name while preserving its visual
styling, using a visually hidden label or role="img" with aria-label. Add the
corresponding artifacts.unseen translation key to all five locale files and use
the existing translation helper.
- Line 332: Update the AutomationRunReplay invocation in App so onOpenArtifacts
uses the existing open-only panel action rather than togglePanel('artifacts'),
matching the jcode:artifact-upserted listener and ensuring repeated clicks keep
the artifacts panel open.
In `@web/src/components/ArtifactsPanel.tsx`:
- Around line 220-231: The fullscreen artifact dialog and share dialog lack
keyboard and focus management despite declaring aria-modal. In
web/src/components/ArtifactsPanel.tsx lines 220-231 and 290-297, extract a
shared modal wrapper that traps focus, moves initial focus to the close button,
handles Escape by invoking setFullscreen(false) for the fullscreen dialog and
onClose for the share dialog, and restores focus to each dialog’s trigger when
closed; apply the wrapper to both sites so they behave identically.
- Around line 62-89: Update parseCSV to return both the parsed rows and a
truncated flag, setting it whenever the maxRows or maxColumns limits omit source
data. Update CSVTable to consume the new result and render a truncation notice
above the table when truncated is true, then add the notice translation to all
five locale files.
- Line 379: Update the retry handling in ArtifactsPanel’s ViewerState error
branch to use local state rather than dispatching the global
jcode:artifact-upserted event. Add a local retry trigger consumed by the
content-fetch effect, include it in that effect’s dependencies, and clear or
replace the existing error when retrying so the artifact content is fetched
again without affecting App-level panel behavior.
- Around line 104-136: Update the ArtifactsPanel load flow to use a useRef-based
request counter, incrementing it for each invocation and applying setRecords,
setSelectedID, setError, and setLoading results only for the latest request. Use
the counter to avoid showing the full loading state during refreshes after the
initial load, while preserving initial loading behavior and cleanup semantics.
Add useRef to the React import and anchor the changes in load and its existing
effects.
In `@web/src/i18n/locales/zh-Hant.ts`:
- Around line 1125-1144: Update the user-visible product name references in
shareDialog, specifically privacy and keyWarning, from “JCode” to the file’s
established “JCODE” casing. Leave the surrounding translations unchanged.
---
Nitpick comments:
In `@internal/cloud/artifact_share.go`:
- Around line 134-142: Update the deferred cleanup in the share operation to
capture the error returned by p.revoke and log any failure through
config.Logger(). Preserve the existing timeout context, cancellation, and
completed guard, while ensuring successful revocations remain silent.
In `@internal/session/artifact_test.go`:
- Around line 43-47: Update the forbidden-token list in the session artifact
test to include “workspace”, so the marshaled entry is explicitly checked for
absence of the workspace absolute path. Keep the existing “<html>”,
“file_content”, and “absolute_path” checks unchanged.
In `@internal/web/models_test.go`:
- Around line 30-34: Strengthen the catalog test around disabledModelID by first
asserting that disabledModelID exists in the catalog response, then asserting
its recorded value is false. Update the check using addedByID so a missing k3
entry cannot pass through its map zero value.
In `@web/src/app/wsBridge.artifact.test.ts`:
- Around line 18-44: Extend the artifact WebSocket bridge tests around
onArtifactUpserted with an active-task event using focus: false and assert that
it emits nothing, then add an active-task event providing id without artifact_id
and assert it emits with artifact_id normalized to that id. Preserve the
existing task-matching and background-artifact coverage.
In `@web/src/components/ArtifactsPanel.tsx`:
- Line 139: Update the viewer rendering in ArtifactsPanel so the inline
ArtifactViewer created by the viewer expression is not mounted while fullscreen
is true. Preserve the fullscreen overlay’s single ArtifactViewer instance and
continue rendering the inline viewer when fullscreen is false.
- Around line 327-331: In ArtifactsPanel, extract the duplicated active-share
predicate into a useMemo-backed value near the component’s other derived state,
using shares as its dependency. Replace both occurrences of the inline
shares.filter expression in the active-share section and map with that memoized
result.
In `@web/src/components/AutomationsView.tsx`:
- Around line 877-883: Update the artifact badge in the AutomationsView render
block to add a title using the existing rightPanel.artifacts translation key,
and mark the unseen indicator dot aria-hidden so it is ignored by screen readers
while preserving its visual meaning.
In `@web/src/lib/api.ts`:
- Around line 40-63: Refactor requestBlob and requestVoid to reuse a shared
raw-Response helper, preferably the existing request helper, for header
construction, bearer-token attachment, fetch execution, 401 notification, and
JSON error handling. Keep requestBlob responsible for decoding the successful
response as a Blob and requestVoid for completing without a body, while removing
their duplicated auth and error-handling logic.
🪄 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 Plus
Run ID: 3d1f7353-e2f1-4d76-a478-5223556aeefb
⛔ Files ignored due to path filters (6)
internal-doc/artifacts-ui/screenshots/01-docked-workbench.pngis excluded by!**/*.pnginternal-doc/artifacts-ui/screenshots/02-focus-canvas.pngis excluded by!**/*.pnginternal-doc/artifacts-ui/screenshots/03-inline-quick-look.pngis excluded by!**/*.pnginternal-doc/artifacts-ui/screenshots/04-logged-out.pngis excluded by!**/*.pnginternal-doc/artifacts-ui/screenshots/05-narrow-web.pngis excluded by!**/*.pnginternal-doc/artifacts-ui/screenshots/06-stale-share.pngis excluded by!**/*.png
📒 Files selected for processing (57)
internal-doc/artifacts-design-review.mdinternal-doc/artifacts-design.mdinternal-doc/artifacts-prd-review.mdinternal-doc/artifacts-prd.mdinternal-doc/artifacts-ui/design-notes.mdinternal-doc/artifacts-ui/index.htmlinternal-doc/artifacts-ui/product-facts.mdinternal-doc/artifacts-ui/prototype.test.cjsinternal/artifact/service.gointernal/artifact/service_test.gointernal/cloud/artifact_share.gointernal/cloud/artifact_share_test.gointernal/command/tool_catalog.gointernal/command/tool_catalog_test.gointernal/command/web.gointernal/command/web_tools_test.gointernal/feature/feature_default.gointernal/feature/feature_desktop.gointernal/handler/web.gointernal/handler/web_display_test.gointernal/runner/approval.gointernal/runner/approval_test.gointernal/session/artifact_test.gointernal/session/session.gointernal/tools/artifact.gointernal/tools/artifact_test.gointernal/web/artifacts.gointernal/web/artifacts_desktop.gointernal/web/artifacts_test.gointernal/web/artifacts_web_test.gointernal/web/automation_api.gointernal/web/automation_api_test.gointernal/web/models_test.gointernal/web/server.gointernal/web/sessions.goweb/src/App.tsxweb/src/app/wsBridge.artifact.test.tsweb/src/app/wsBridge.tsweb/src/components/ArtifactsPanel.test.tsxweb/src/components/ArtifactsPanel.tsxweb/src/components/AutomationsView.tsxweb/src/components/DesktopTitlebar.tsxweb/src/components/RightPanel.tsxweb/src/components/TopBar.tsxweb/src/i18n/locales/en.tsweb/src/i18n/locales/ja.tsweb/src/i18n/locales/ko.tsweb/src/i18n/locales/zh-Hans.tsweb/src/i18n/locales/zh-Hant.tsweb/src/lib/api.tsweb/src/lib/automation.tsweb/src/lib/toolInfo.artifact.test.tsweb/src/lib/toolInfo.tsweb/src/lib/types.tsweb/src/lib/ws.artifact.test.tsweb/src/lib/ws.tsweb/src/styles.css
|
|
||
| Web 只能通过 Artifact ID 获取经过再次校验的内容，不接受任意绝对路径。每次 content/download/open 都重新验证 session ownership、canonical containment、regular file 和 symlink，Artifact ID 不是永久授权。 | ||
|
|
||
| 安全预览的最低合同：HTML iframe 仅允许 `sandbox="allow-scripts"`，禁止 `allow-same-origin`、form、popup、top navigation 和联网；SVG 只能作为 image document；Markdown 默认禁用 raw HTML；不得把 Artifact HTML/SVG 注入主 React DOM；内容响应必须带 `X-Content-Type-Options: nosniff` 和对应 CSP。`.git/**`、`.jcode/**`、`.env`、private key 等敏感路径必须拒绝，Artifact 不能成为 Files 权限旁路。 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Specify a network-denying CSP for HTML artifacts.
sandbox="allow-scripts" does not block external resource requests. A generic “corresponding CSP” can still permit remote scripts, images, stylesheets, or fetch calls.
Require a server-generated response header with explicit permitted sources, such as default-src 'none', and only the required local schemes for scripts, styles, images, and fonts. Add negative tests for remote script, img, link, fetch, and form submissions.
🤖 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 `@internal-doc/artifacts-prd.md` at line 237, Update the “安全预览的最低合同”
specification to require a server-generated CSP that denies network access by
default, using explicit local-only sources for any required scripts, styles,
images, and fonts. Add negative tests covering remote script, image, stylesheet,
fetch, and form-submission attempts, while preserving the existing iframe
sandbox and sensitive-path restrictions.
Comment on lines
+966
to
+981
| document.querySelector('.panel-close').addEventListener('click', () => { app.style.gridTemplateColumns = '264px minmax(0, 1fr) 0'; showToast('Artifacts panel closed · reopen with ⇧⌘A'); }); | ||
| shareButton.addEventListener('click', () => { shareOpen = !shareOpen; renderShare(); }); | ||
| fullscreenShare.addEventListener('click', () => { fullscreen.classList.remove('open'); shareOpen = true; setVariant('docked'); renderShare(); }); | ||
| loginToggle.addEventListener('change', renderShare); | ||
| shareState.addEventListener('change', () => { shareOpen = true; renderShare(); }); | ||
| sharePopover.addEventListener('click', (event) => { | ||
| if (event.target.closest('.close-share')) { shareOpen = false; renderShare(); } | ||
| if (event.target.closest('.start-share, .share-latest')) { shareState.value = 'uploading'; renderShare(); } | ||
| if (event.target.closest('.cancel-upload')) { shareState.value = 'unshared'; renderShare(); } | ||
| if (event.target.closest('.revoke-share')) { shareState.value = 'revoked'; renderShare(); } | ||
| if (event.target.closest('.copy-link')) showToast('Encrypted share link copied'); | ||
| }); | ||
| document.addEventListener('keydown', (event) => { | ||
| if ((event.metaKey || event.ctrlKey) && event.shiftKey && event.key.toLowerCase() === 'a') { event.preventDefault(); setVariant('docked'); } | ||
| if (event.key === 'Escape') { fullscreen.classList.remove('open'); shareOpen = false; renderShare(); } | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix the ⇧⌘A shortcut so it actually reopens the closed Artifacts panel.
The panel-close handler sets an inline style, app.style.gridTemplateColumns = '264px minmax(0, 1fr) 0', to collapse the panel. The toast on that same line tells the user to reopen the panel with ⇧⌘A. The keydown handler only calls setVariant('docked'), which sets data-variant and aria-pressed attributes. It never clears the inline gridTemplateColumns style. Because the inline style has higher specificity than the CSS grid-template-columns rules, the panel stays collapsed even after ⇧⌘A. Reset the inline style in the shortcut handler.
🐛 Proposed fix to restore the panel on ⇧⌘A
document.addEventListener('keydown', (event) => {
- if ((event.metaKey || event.ctrlKey) && event.shiftKey && event.key.toLowerCase() === 'a') { event.preventDefault(); setVariant('docked'); }
+ if ((event.metaKey || event.ctrlKey) && event.shiftKey && event.key.toLowerCase() === 'a') {
+ event.preventDefault();
+ app.style.gridTemplateColumns = '';
+ setVariant('docked');
+ }
if (event.key === 'Escape') { fullscreen.classList.remove('open'); shareOpen = false; renderShare(); }
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| document.querySelector('.panel-close').addEventListener('click', () => { app.style.gridTemplateColumns = '264px minmax(0, 1fr) 0'; showToast('Artifacts panel closed · reopen with ⇧⌘A'); }); | |
| shareButton.addEventListener('click', () => { shareOpen = !shareOpen; renderShare(); }); | |
| fullscreenShare.addEventListener('click', () => { fullscreen.classList.remove('open'); shareOpen = true; setVariant('docked'); renderShare(); }); | |
| loginToggle.addEventListener('change', renderShare); | |
| shareState.addEventListener('change', () => { shareOpen = true; renderShare(); }); | |
| sharePopover.addEventListener('click', (event) => { | |
| if (event.target.closest('.close-share')) { shareOpen = false; renderShare(); } | |
| if (event.target.closest('.start-share, .share-latest')) { shareState.value = 'uploading'; renderShare(); } | |
| if (event.target.closest('.cancel-upload')) { shareState.value = 'unshared'; renderShare(); } | |
| if (event.target.closest('.revoke-share')) { shareState.value = 'revoked'; renderShare(); } | |
| if (event.target.closest('.copy-link')) showToast('Encrypted share link copied'); | |
| }); | |
| document.addEventListener('keydown', (event) => { | |
| if ((event.metaKey || event.ctrlKey) && event.shiftKey && event.key.toLowerCase() === 'a') { event.preventDefault(); setVariant('docked'); } | |
| if (event.key === 'Escape') { fullscreen.classList.remove('open'); shareOpen = false; renderShare(); } | |
| }); | |
| document.querySelector('.panel-close').addEventListener('click', () => { app.style.gridTemplateColumns = '264px minmax(0, 1fr) 0'; showToast('Artifacts panel closed · reopen with ⇧⌘A'); }); | |
| shareButton.addEventListener('click', () => { shareOpen = !shareOpen; renderShare(); }); | |
| fullscreenShare.addEventListener('click', () => { fullscreen.classList.remove('open'); shareOpen = true; setVariant('docked'); renderShare(); }); | |
| loginToggle.addEventListener('change', renderShare); | |
| shareState.addEventListener('change', () => { shareOpen = true; renderShare(); }); | |
| sharePopover.addEventListener('click', (event) => { | |
| if (event.target.closest('.close-share')) { shareOpen = false; renderShare(); } | |
| if (event.target.closest('.start-share, .share-latest')) { shareState.value = 'uploading'; renderShare(); } | |
| if (event.target.closest('.cancel-upload')) { shareState.value = 'unshared'; renderShare(); } | |
| if (event.target.closest('.revoke-share')) { shareState.value = 'revoked'; renderShare(); } | |
| if (event.target.closest('.copy-link')) showToast('Encrypted share link copied'); | |
| }); | |
| document.addEventListener('keydown', (event) => { | |
| if ((event.metaKey || event.ctrlKey) && event.shiftKey && event.key.toLowerCase() === 'a') { | |
| event.preventDefault(); | |
| app.style.gridTemplateColumns = ''; | |
| setVariant('docked'); | |
| } | |
| if (event.key === 'Escape') { fullscreen.classList.remove('open'); shareOpen = false; renderShare(); } | |
| }); |
🤖 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 `@internal-doc/artifacts-ui/index.html` around lines 966 - 981, Update the ⇧⌘A
shortcut in the document keydown handler to clear the inline
app.style.gridTemplateColumns value before or alongside setVariant('docked'),
allowing the docked CSS layout to restore the Artifacts panel. Preserve the
existing preventDefault and variant behavior.
Comment on lines
+418
to
+445
| func classifyFile(path string, hint Kind) (Kind, string, error) { | ||
| if hint == "" { | ||
| hint = KindAuto | ||
| } | ||
| if !validKind(hint) { | ||
| return "", "", fmt.Errorf("unsupported artifact kind %q", hint) | ||
| } | ||
| ext := strings.ToLower(filepath.Ext(path)) | ||
| mediaType := mime.TypeByExtension(ext) | ||
| file, err := os.Open(path) | ||
| if err != nil { | ||
| return "", "", err | ||
| } | ||
| defer func() { _ = file.Close() }() | ||
| var sample [512]byte | ||
| n, readErr := file.Read(sample[:]) | ||
| if readErr != nil && !errors.Is(readErr, io.EOF) && n == 0 { | ||
| return "", "", readErr | ||
| } | ||
| if mediaType == "" { | ||
| mediaType = http.DetectContentType(sample[:n]) | ||
| } | ||
| detected := kindForExtension(ext, mediaType) | ||
| if hint != KindAuto { | ||
| detected = hint | ||
| } | ||
| return detected, strings.Split(mediaType, ";")[0], nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | sed -n '1,120p'
echo "== locate target files =="
fd -a 'service.go|artifacts-design.md|go.mod' . | sed 's#^\./##' || true
echo "== target service.go lines =="
if [ -f internal/artifact/service.go ]; then
wc -l internal/artifact/service.go
sed -n '380,460p' internal/artifact/service.go
fi
echo "== design doc snippets =="
for f in $(fd 'artifacts-design.md' .); do
echo "--- $f"
wc -l "$f"
rg -n -C 4 'kind|sort|服务端|hint|extension|嗅探|sniff|xss|renderer|10\.3|7\.3' "$f" || true
done
echo "== go directive =="
for f in $(fd 'go.mod' .); do
echo "--- $f"
sed -n '1,20p' "$f"
done
echo "== search classifyFile usages =="
rg -n -C 3 'classifyFile|RecordArtifact|KindAuto|validKind|kindForExtension' .Repository: cnjack/jcode
Length of output: 33785
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== web fixtures and kind handling =="
for f in internal/web/artifacts.go internal/web/artifacts_test.go internal/tools/artifact.go internal/tools/artifact_test.go; do
if [ -f "$f" ]; then
echo "--- $f"
wc -l "$f"
rg -n -C 4 'show_artifact|Kind|kind|Content-Type|X-Content-Type-Options|artifacts/|RecordArtifact|ShowArtifact' "$f" || true
fi
done
echo "== frontend renderer hints =="
rg -n -C 3 'artifact_kind|artifactKind|kind routing|renderer|artifacts' console src -g '*.{ts,tsx,jsx,js}' 2>/dev/null || true
echo "== classifyFile tests =="
sed -n '1,260p' internal/artifact/service_test.go
sed -n '220,340p' internal/web/artifacts_test.go
sed -n '1,220p' internal/tools/artifact_test.goRepository: cnjack/jcode
Length of output: 24594
Do not let kind override the server-detected artifact kind.
classifyFile replaces the extension/sniff result with any non-KindAuto hint, but internal-doc/artifacts-design.md defines kind as only an Agent hint while extension plus sniff determine the server kind. Keep client kind as a hint/narrowing only, or ignore it so Record.Kind remains server-authoritative.
🤖 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 `@internal/artifact/service.go` around lines 418 - 445, Update classifyFile so
the detected kind from kindForExtension remains authoritative and a non-KindAuto
hint cannot replace it. Treat hint only as optional narrowing/validation if
supported by the existing contract; otherwise ignore it while preserving
validation of supported values and leaving media-type detection unchanged.
Comment on lines
+87
to
+102
| mux.HandleFunc("POST /internal/v1/device/artifact-shares/intents", func(w http.ResponseWriter, r *http.Request) { | ||
| if r.Header.Get("Authorization") != "Bearer device-token" { | ||
| t.Fatalf("intent authorization = %q", r.Header.Get("Authorization")) | ||
| } | ||
| body, _ := io.ReadAll(r.Body) | ||
| mu.Lock() | ||
| intentBody = body | ||
| mu.Unlock() | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(http.StatusCreated) | ||
| _, _ = io.WriteString(w, `{"share_id":"share-123","upload_url":"/internal/v1/device/artifact-shares/share-123/content","complete_url":"/internal/v1/device/artifact-shares/share-123/complete","base_url":"https://share.example/s/share-123","expires_at":"2026-08-08T00:00:00Z"}`) | ||
| }) | ||
| mux.HandleFunc("PUT /internal/v1/device/artifact-shares/share-123/content", func(w http.ResponseWriter, r *http.Request) { | ||
| if r.Header.Get("Authorization") != "Bearer device-token" { | ||
| t.Fatalf("upload authorization = %q", r.Header.Get("Authorization")) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not call t.Fatalf from HTTP handler goroutines.
httptest runs each handler in its own goroutine. t.Fatalf calls runtime.Goexit on the calling goroutine, so it aborts the handler instead of the test. The client then sees a truncated or empty response, and the failure reason is lost. Use t.Errorf and return an error status instead.
🐛 Proposed fix
mux.HandleFunc("POST /internal/v1/device/artifact-shares/intents", func(w http.ResponseWriter, r *http.Request) {
if r.Header.Get("Authorization") != "Bearer device-token" {
- t.Fatalf("intent authorization = %q", r.Header.Get("Authorization"))
+ t.Errorf("intent authorization = %q", r.Header.Get("Authorization"))
+ http.Error(w, "unauthorized", http.StatusUnauthorized)
+ return
}
@@
mux.HandleFunc("PUT /internal/v1/device/artifact-shares/share-123/content", func(w http.ResponseWriter, r *http.Request) {
if r.Header.Get("Authorization") != "Bearer device-token" {
- t.Fatalf("upload authorization = %q", r.Header.Get("Authorization"))
+ t.Errorf("upload authorization = %q", r.Header.Get("Authorization"))
+ http.Error(w, "unauthorized", http.StatusUnauthorized)
+ return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| mux.HandleFunc("POST /internal/v1/device/artifact-shares/intents", func(w http.ResponseWriter, r *http.Request) { | |
| if r.Header.Get("Authorization") != "Bearer device-token" { | |
| t.Fatalf("intent authorization = %q", r.Header.Get("Authorization")) | |
| } | |
| body, _ := io.ReadAll(r.Body) | |
| mu.Lock() | |
| intentBody = body | |
| mu.Unlock() | |
| w.Header().Set("Content-Type", "application/json") | |
| w.WriteHeader(http.StatusCreated) | |
| _, _ = io.WriteString(w, `{"share_id":"share-123","upload_url":"/internal/v1/device/artifact-shares/share-123/content","complete_url":"/internal/v1/device/artifact-shares/share-123/complete","base_url":"https://share.example/s/share-123","expires_at":"2026-08-08T00:00:00Z"}`) | |
| }) | |
| mux.HandleFunc("PUT /internal/v1/device/artifact-shares/share-123/content", func(w http.ResponseWriter, r *http.Request) { | |
| if r.Header.Get("Authorization") != "Bearer device-token" { | |
| t.Fatalf("upload authorization = %q", r.Header.Get("Authorization")) | |
| } | |
| mux.HandleFunc("POST /internal/v1/device/artifact-shares/intents", func(w http.ResponseWriter, r *http.Request) { | |
| if r.Header.Get("Authorization") != "Bearer device-token" { | |
| t.Errorf("intent authorization = %q", r.Header.Get("Authorization")) | |
| http.Error(w, "unauthorized", http.StatusUnauthorized) | |
| return | |
| } | |
| body, _ := io.ReadAll(r.Body) | |
| mu.Lock() | |
| intentBody = body | |
| mu.Unlock() | |
| w.Header().Set("Content-Type", "application/json") | |
| w.WriteHeader(http.StatusCreated) | |
| _, _ = io.WriteString(w, `{"share_id":"share-123","upload_url":"/internal/v1/device/artifact-shares/share-123/content","complete_url":"/internal/v1/device/artifact-shares/share-123/complete","base_url":"https://share.example/s/share-123","expires_at":"2026-08-08T00:00:00Z"}`) | |
| }) | |
| mux.HandleFunc("PUT /internal/v1/device/artifact-shares/share-123/content", func(w http.ResponseWriter, r *http.Request) { | |
| if r.Header.Get("Authorization") != "Bearer device-token" { | |
| t.Errorf("upload authorization = %q", r.Header.Get("Authorization")) | |
| http.Error(w, "unauthorized", http.StatusUnauthorized) | |
| return | |
| } |
🤖 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 `@internal/cloud/artifact_share_test.go` around lines 87 - 102, Replace the
t.Fatalf authorization assertions in the intent and upload HTTP handlers with
t.Errorf, then return an appropriate error response immediately when
authorization is invalid. Keep the valid-authorization behavior unchanged so
handlers always produce a complete response before returning.
Comment on lines
+189
to
+212
| func TestArtifactSharePublisherRevokesIntentWhenUploadFails(t *testing.T) { | ||
| t.Parallel() | ||
| revoked := false | ||
| mux := http.NewServeMux() | ||
| mux.HandleFunc("POST /internal/v1/device/artifact-shares/intents", func(w http.ResponseWriter, _ *http.Request) { | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(http.StatusCreated) | ||
| _, _ = io.WriteString(w, `{"share_id":"share-fail","upload_url":"/internal/v1/device/artifact-shares/share-fail/content","complete_url":"/internal/v1/device/artifact-shares/share-fail/complete","base_url":"https://share.example/s/share-fail","expires_at":"2026-08-08T00:00:00Z"}`) | ||
| }) | ||
| mux.HandleFunc("PUT /internal/v1/device/artifact-shares/share-fail/content", func(w http.ResponseWriter, _ *http.Request) { http.Error(w, "failed", http.StatusBadGateway) }) | ||
| mux.HandleFunc("DELETE /internal/v1/device/artifact-shares/share-fail", func(w http.ResponseWriter, _ *http.Request) { revoked = true; w.WriteHeader(http.StatusNoContent) }) | ||
| server := httptest.NewServer(mux) | ||
| defer server.Close() | ||
| publisher := NewArtifactSharePublisher(server.Client()) | ||
| _, err := publisher.Publish(context.Background(), &Credentials{CloudURL: server.URL, DeviceToken: "token"}, ArtifactShareInput{ | ||
| ArtifactID: "artifact", Revision: 1, Title: "x", MediaType: "text/plain", Kind: "text", Content: []byte("x"), ExpiresIn: time.Hour, | ||
| }) | ||
| if err == nil { | ||
| t.Fatal("Publish unexpectedly succeeded") | ||
| } | ||
| if !revoked { | ||
| t.Fatal("failed upload did not revoke the incomplete intent") | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard revoked against a data race.
The DELETE handler writes revoked on the server goroutine. Line 209 reads it on the test goroutine. No synchronization links the two, so go test -race can report a race and the read can observe a stale value. The sibling test already uses a sync.Mutex for the same pattern. Apply the same protection here.
🔒 Proposed fix
t.Parallel()
- revoked := false
+ var mu sync.Mutex
+ revoked := false
mux := http.NewServeMux()
@@
- mux.HandleFunc("DELETE /internal/v1/device/artifact-shares/share-fail", func(w http.ResponseWriter, _ *http.Request) { revoked = true; w.WriteHeader(http.StatusNoContent) })
+ mux.HandleFunc("DELETE /internal/v1/device/artifact-shares/share-fail", func(w http.ResponseWriter, _ *http.Request) {
+ mu.Lock()
+ revoked = true
+ mu.Unlock()
+ w.WriteHeader(http.StatusNoContent)
+ })
@@
- if !revoked {
+ mu.Lock()
+ defer mu.Unlock()
+ if !revoked {
t.Fatal("failed upload did not revoke the incomplete intent")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestArtifactSharePublisherRevokesIntentWhenUploadFails(t *testing.T) { | |
| t.Parallel() | |
| revoked := false | |
| mux := http.NewServeMux() | |
| mux.HandleFunc("POST /internal/v1/device/artifact-shares/intents", func(w http.ResponseWriter, _ *http.Request) { | |
| w.Header().Set("Content-Type", "application/json") | |
| w.WriteHeader(http.StatusCreated) | |
| _, _ = io.WriteString(w, `{"share_id":"share-fail","upload_url":"/internal/v1/device/artifact-shares/share-fail/content","complete_url":"/internal/v1/device/artifact-shares/share-fail/complete","base_url":"https://share.example/s/share-fail","expires_at":"2026-08-08T00:00:00Z"}`) | |
| }) | |
| mux.HandleFunc("PUT /internal/v1/device/artifact-shares/share-fail/content", func(w http.ResponseWriter, _ *http.Request) { http.Error(w, "failed", http.StatusBadGateway) }) | |
| mux.HandleFunc("DELETE /internal/v1/device/artifact-shares/share-fail", func(w http.ResponseWriter, _ *http.Request) { revoked = true; w.WriteHeader(http.StatusNoContent) }) | |
| server := httptest.NewServer(mux) | |
| defer server.Close() | |
| publisher := NewArtifactSharePublisher(server.Client()) | |
| _, err := publisher.Publish(context.Background(), &Credentials{CloudURL: server.URL, DeviceToken: "token"}, ArtifactShareInput{ | |
| ArtifactID: "artifact", Revision: 1, Title: "x", MediaType: "text/plain", Kind: "text", Content: []byte("x"), ExpiresIn: time.Hour, | |
| }) | |
| if err == nil { | |
| t.Fatal("Publish unexpectedly succeeded") | |
| } | |
| if !revoked { | |
| t.Fatal("failed upload did not revoke the incomplete intent") | |
| } | |
| } | |
| func TestArtifactSharePublisherRevokesIntentWhenUploadFails(t *testing.T) { | |
| t.Parallel() | |
| var mu sync.Mutex | |
| revoked := false | |
| mux := http.NewServeMux() | |
| mux.HandleFunc("POST /internal/v1/device/artifact-shares/intents", func(w http.ResponseWriter, _ *http.Request) { | |
| w.Header().Set("Content-Type", "application/json") | |
| w.WriteHeader(http.StatusCreated) | |
| _, _ = io.WriteString(w, `{"share_id":"share-fail","upload_url":"/internal/v1/device/artifact-shares/share-fail/content","complete_url":"/internal/v1/device/artifact-shares/share-fail/complete","base_url":"https://share.example/s/share-fail","expires_at":"2026-08-08T00:00:00Z"}`) | |
| }) | |
| mux.HandleFunc("PUT /internal/v1/device/artifact-shares/share-fail/content", func(w http.ResponseWriter, _ *http.Request) { http.Error(w, "failed", http.StatusBadGateway) }) | |
| mux.HandleFunc("DELETE /internal/v1/device/artifact-shares/share-fail", func(w http.ResponseWriter, _ *http.Request) { | |
| mu.Lock() | |
| revoked = true | |
| mu.Unlock() | |
| w.WriteHeader(http.StatusNoContent) | |
| }) | |
| server := httptest.NewServer(mux) | |
| defer server.Close() | |
| publisher := NewArtifactSharePublisher(server.Client()) | |
| _, err := publisher.Publish(context.Background(), &Credentials{CloudURL: server.URL, DeviceToken: "token"}, ArtifactShareInput{ | |
| ArtifactID: "artifact", Revision: 1, Title: "x", MediaType: "text/plain", Kind: "text", Content: []byte("x"), ExpiresIn: time.Hour, | |
| }) | |
| if err == nil { | |
| t.Fatal("Publish unexpectedly succeeded") | |
| } | |
| mu.Lock() | |
| defer mu.Unlock() | |
| if !revoked { | |
| t.Fatal("failed upload did not revoke the incomplete intent") | |
| } | |
| } |
🤖 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 `@internal/cloud/artifact_share_test.go` around lines 189 - 212, Protect the
shared revoked state in TestArtifactSharePublisherRevokesIntentWhenUploadFails
with a sync.Mutex, matching the sibling test: lock around the DELETE handler’s
write and around the final assertion’s read, and add the required sync import if
absent.
Comment on lines
+62
to
+89
| function parseCSV(source: string, maxRows = 200, maxColumns = 50): string[][] { | ||
| const rows: string[][] = [] | ||
| let row: string[] = [] | ||
| let field = '' | ||
| let quoted = false | ||
| for (let i = 0; i < source.length && rows.length < maxRows; i++) { | ||
| const char = source[i] | ||
| if (char === '"') { | ||
| if (quoted && source[i + 1] === '"') { field += '"'; i++ } else quoted = !quoted | ||
| } else if (char === ',' && !quoted) { | ||
| if (row.length < maxColumns) row.push(field) | ||
| field = '' | ||
| } else if ((char === '\n' || char === '\r') && !quoted) { | ||
| if (char === '\r' && source[i + 1] === '\n') i++ | ||
| if (row.length < maxColumns) row.push(field) | ||
| rows.push(row) | ||
| row = [] | ||
| field = '' | ||
| } else { | ||
| field += char | ||
| } | ||
| } | ||
| if ((field || row.length) && rows.length < maxRows) { | ||
| if (row.length < maxColumns) row.push(field) | ||
| rows.push(row) | ||
| } | ||
| return rows | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
parseCSV truncates silently.
parseCSV stops at 200 rows and drops every field past column 50. CSVTable renders the result with no notice, so a user sees a complete-looking table that omits data. Return the truncation state and render a notice above the table.
♻️ Proposed change
-function parseCSV(source: string, maxRows = 200, maxColumns = 50): string[][] {
+function parseCSV(source: string, maxRows = 200, maxColumns = 50): { rows: string[][]; truncated: boolean } {
const rows: string[][] = []
let row: string[] = []
let field = ''
let quoted = false
- for (let i = 0; i < source.length && rows.length < maxRows; i++) {
+ let truncated = false
+ let i = 0
+ for (; i < source.length && rows.length < maxRows; i++) {
const char = source[i]
if (char === '"') {
if (quoted && source[i + 1] === '"') { field += '"'; i++ } else quoted = !quoted
} else if (char === ',' && !quoted) {
- if (row.length < maxColumns) row.push(field)
+ if (row.length < maxColumns) row.push(field); else truncated = true
field = ''
} else if ((char === '\n' || char === '\r') && !quoted) {
if (char === '\r' && source[i + 1] === '\n') i++
- if (row.length < maxColumns) row.push(field)
+ if (row.length < maxColumns) row.push(field); else truncated = true
rows.push(row)
row = []
field = ''
} else {
field += char
}
}
+ if (i < source.length) truncated = true
if ((field || row.length) && rows.length < maxRows) {
if (row.length < maxColumns) row.push(field)
rows.push(row)
}
- return rows
+ return { rows, truncated }
}Update CSVTable to read rows and render a truncation notice when truncated is true. Add the notice string to all five locale files.
🤖 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 `@web/src/components/ArtifactsPanel.tsx` around lines 62 - 89, Update parseCSV
to return both the parsed rows and a truncated flag, setting it whenever the
maxRows or maxColumns limits omit source data. Update CSVTable to consume the
new result and render a truncation notice above the table when truncated is
true, then add the notice translation to all five locale files.
Comment on lines
+104
to
+136
| const load = useCallback(async (preferredID?: string) => { | ||
| if (!taskId) { setRecords([]); setLoading(false); return } | ||
| setLoading(true) | ||
| setError('') | ||
| try { | ||
| const next = await api.artifacts(taskId) | ||
| setRecords(next) | ||
| setSelectedID((current) => { | ||
| if (preferredID && next.some((item) => item.id === preferredID)) return preferredID | ||
| return next.some((item) => item.id === current) ? current : (next[0]?.id ?? '') | ||
| }) | ||
| await api.markArtifactsViewed(taskId).catch(() => undefined) | ||
| } catch { | ||
| setError(t('artifacts.loadError')) | ||
| } finally { | ||
| setLoading(false) | ||
| } | ||
| }, [taskId, t]) | ||
|
|
||
| useEffect(() => { void load() }, [load]) | ||
| useEffect(() => { | ||
| let active = true | ||
| void api.cloudStatus().then((status) => { if (active) setCloudLoggedIn(status.logged_in) }).catch(() => undefined) | ||
| return () => { active = false } | ||
| }, []) | ||
| useEffect(() => { | ||
| const refresh = (event: Event) => { | ||
| const detail = (event as CustomEvent<{ artifact_id?: string }>).detail | ||
| void load(detail?.artifact_id) | ||
| } | ||
| window.addEventListener('jcode:artifact-upserted', refresh) | ||
| return () => window.removeEventListener('jcode:artifact-upserted', refresh) | ||
| }, [load]) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
load has no cancellation guard, so overlapping refreshes can apply a stale response.
load runs on mount and on every jcode:artifact-upserted event. Two events in quick succession start two api.artifacts calls with no ordering guarantee. Whichever settles last wins, so an older response can overwrite a newer list. The other effects in this file (Lines 124-128, 350-357, 361-373) all use an active flag; load does not.
A request counter fixes it and also lets you skip the full loading state on refresh, which currently makes the list flash on every upsert.
🐛 Proposed fix
+ const requestID = useRef(0)
const load = useCallback(async (preferredID?: string) => {
if (!taskId) { setRecords([]); setLoading(false); return }
+ const current = ++requestID.current
setLoading(true)
setError('')
try {
const next = await api.artifacts(taskId)
+ if (current !== requestID.current) return
setRecords(next)
setSelectedID((current) => {
if (preferredID && next.some((item) => item.id === preferredID)) return preferredID
return next.some((item) => item.id === current) ? current : (next[0]?.id ?? '')
})
await api.markArtifactsViewed(taskId).catch(() => undefined)
} catch {
+ if (current !== requestID.current) return
setError(t('artifacts.loadError'))
} finally {
- setLoading(false)
+ if (current === requestID.current) setLoading(false)
}
}, [taskId, t])Add useRef to the React import.
🤖 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 `@web/src/components/ArtifactsPanel.tsx` around lines 104 - 136, Update the
ArtifactsPanel load flow to use a useRef-based request counter, incrementing it
for each invocation and applying setRecords, setSelectedID, setError, and
setLoading results only for the latest request. Use the counter to avoid showing
the full loading state during refreshes after the initial load, while preserving
initial loading behavior and cleanup semantics. Add useRef to the React import
and anchor the changes in load and its existing effects.
Comment on lines
+220
to
+231
| {fullscreen && selected && ( | ||
| <div role="dialog" aria-modal="true" aria-label={selected.title} className="fixed inset-3 z-[var(--z-modal)] flex flex-col overflow-hidden rounded-[var(--radius-xl)] border border-[var(--color-border)] bg-[var(--color-background)] shadow-[var(--shadow-xl)]"> | ||
| <header className="flex h-12 shrink-0 items-center border-b border-[var(--color-border)] px-4"> | ||
| <h2 className="min-w-0 flex-1 truncate text-sm font-semibold">{selected.title}</h2> | ||
| <button type="button" aria-label={t('common.close')} onClick={() => setFullscreen(false)} className="artifact-action"><XMarkIcon className="h-4 w-4" /></button> | ||
| </header> | ||
| <div className="min-h-0 flex-1 overflow-auto p-5"><ArtifactViewer taskId={taskId} record={selected} /></div> | ||
| </div> | ||
| )} | ||
| {shareOpen && selected && ( | ||
| <ArtifactShareDialog key={`${selected.id}:${selected.revision}`} taskId={taskId} record={selected} onClose={() => setShareOpen(false)} /> | ||
| )} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Both artifact dialogs declare aria-modal="true" without implementing modal keyboard behavior. The shared root cause is that each dialog is a plain positioned div with modal ARIA attributes and no focus management. aria-modal="true" tells assistive technology that background content is inert, but Tab still reaches the artifact list and the header buttons, Escape does not close either dialog, focus is not moved into the dialog on open, and focus is not restored to the trigger on close.
web/src/components/ArtifactsPanel.tsx#L220-L231: add focus trapping, an Escape key handler that callssetFullscreen(false), initial focus on the close button, and focus restore to the fullscreen trigger.web/src/components/ArtifactsPanel.tsx#L290-L297: apply the same treatment, with Escape callingonCloseand focus restore to the share trigger.
Extract one shared modal wrapper so both dialogs get identical behavior.
📍 Affects 1 file
web/src/components/ArtifactsPanel.tsx#L220-L231(this comment)web/src/components/ArtifactsPanel.tsx#L290-L297
🤖 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 `@web/src/components/ArtifactsPanel.tsx` around lines 220 - 231, The fullscreen
artifact dialog and share dialog lack keyboard and focus management despite
declaring aria-modal. In web/src/components/ArtifactsPanel.tsx lines 220-231 and
290-297, extract a shared modal wrapper that traps focus, moves initial focus to
the close button, handles Escape by invoking setFullscreen(false) for the
fullscreen dialog and onClose for the share dialog, and restores focus to each
dialog’s trigger when closed; apply the wrapper to both sites so they behave
identically.
| useEffect(() => () => { if (objectURL) URL.revokeObjectURL(objectURL) }, [objectURL]) | ||
|
|
||
| if (record.status !== 'available') return <ViewerState title={t(`artifacts.status.${record.status}`)} /> | ||
| if (error) return <ViewerState title={error} retry={() => window.dispatchEvent(new Event('jcode:artifact-upserted'))} /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The Retry button does not retry the content fetch.
Retry dispatches a bare jcode:artifact-upserted event. The panel handler at Lines 129-136 calls load(undefined), which refetches the artifact list. The content effect at Lines 350-357 depends on [record.id, record.status, taskId, t]. A reload that returns the same artifact leaves record.id and record.status unchanged, so the effect does not re-run, error stays set, and the viewer keeps showing the error. The user clicks Retry and nothing changes.
The event name is also overloaded. It signals "the server upserted a focused artifact" and force-opens the artifacts panel in web/src/App.tsx at Lines 246-253. A local retry click should not broadcast that signal.
Drive the retry from local state instead.
🐛 Proposed fix
+ const [attempt, setAttempt] = useState(0)
useEffect(() => {
let active = true
setBlob(null)
setError('')
if (record.status !== 'available') return () => { active = false }
void api.artifactContent(taskId, record.id).then((next) => { if (active) setBlob(next) }).catch(() => { if (active) setError(t('artifacts.contentError')) })
return () => { active = false }
- }, [record.id, record.status, taskId, t])
+ }, [record.id, record.status, taskId, t, attempt])- if (error) return <ViewerState title={error} retry={() => window.dispatchEvent(new Event('jcode:artifact-upserted'))} />
+ if (error) return <ViewerState title={error} retry={() => setAttempt((n) => n + 1)} />🤖 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 `@web/src/components/ArtifactsPanel.tsx` at line 379, Update the retry handling
in ArtifactsPanel’s ViewerState error branch to use local state rather than
dispatching the global jcode:artifact-upserted event. Add a local retry trigger
consumed by the content-fetch effect, include it in that effect’s dependencies,
and clear or replace the existing error when retrying so the artifact content is
fetched again without affecting App-level panel behavior.
Comment on lines
+1125
to
+1144
| shareDialog: { | ||
| title: '分享加密產物', | ||
| privacy: 'JCode 會在本機加密檔案及其中繼資料。Cloud 只儲存密文；解密金鑰僅存在於連結的 fragment 中。', | ||
| expiry: '連結有效期', | ||
| oneHour: '1 小時', | ||
| oneDay: '1 天', | ||
| sevenDays: '7 天', | ||
| thirtyDays: '30 天', | ||
| create: '建立加密連結', | ||
| creating: '正在加密並上傳…', | ||
| createError: '無法建立加密連結。', | ||
| ready: '加密連結已就緒', | ||
| link: '加密產物連結', | ||
| copy: '複製連結', | ||
| open: '開啟', | ||
| keyWarning: '請立即複製此連結。JCode 不會保留其中的 fragment 金鑰。', | ||
| active: '有效連結', | ||
| revoke: '撤銷連結', | ||
| revokeError: '無法撤銷此連結。', | ||
| }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the same product name casing as the rest of the file.
Lines 1127 and 1140 write "JCode". Every other user-visible string in this file writes "JCODE", for example line 183 and line 1011. Align the casing so the product name stays consistent.
✏️ Proposed fix
- privacy: 'JCode 會在本機加密檔案及其中繼資料。Cloud 只儲存密文；解密金鑰僅存在於連結的 fragment 中。',
+ privacy: 'JCODE 會在本機加密檔案及其中繼資料。Cloud 只儲存密文；解密金鑰僅存在於連結的 fragment 中。',
@@
- keyWarning: '請立即複製此連結。JCode 不會保留其中的 fragment 金鑰。',
+ keyWarning: '請立即複製此連結。JCODE 不會保留其中的 fragment 金鑰。',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| shareDialog: { | |
| title: '分享加密產物', | |
| privacy: 'JCode 會在本機加密檔案及其中繼資料。Cloud 只儲存密文；解密金鑰僅存在於連結的 fragment 中。', | |
| expiry: '連結有效期', | |
| oneHour: '1 小時', | |
| oneDay: '1 天', | |
| sevenDays: '7 天', | |
| thirtyDays: '30 天', | |
| create: '建立加密連結', | |
| creating: '正在加密並上傳…', | |
| createError: '無法建立加密連結。', | |
| ready: '加密連結已就緒', | |
| link: '加密產物連結', | |
| copy: '複製連結', | |
| open: '開啟', | |
| keyWarning: '請立即複製此連結。JCode 不會保留其中的 fragment 金鑰。', | |
| active: '有效連結', | |
| revoke: '撤銷連結', | |
| revokeError: '無法撤銷此連結。', | |
| }, | |
| shareDialog: { | |
| title: '分享加密產物', | |
| privacy: 'JCODE 會在本機加密檔案及其中繼資料。Cloud 只儲存密文；解密金鑰僅存在於連結的 fragment 中。', | |
| expiry: '連結有效期', | |
| oneHour: '1 小時', | |
| oneDay: '1 天', | |
| sevenDays: '7 天', | |
| thirtyDays: '30 天', | |
| create: '建立加密連結', | |
| creating: '正在加密並上傳…', | |
| createError: '無法建立加密連結。', | |
| ready: '加密連結已就緒', | |
| link: '加密產物連結', | |
| copy: '複製連結', | |
| open: '開啟', | |
| keyWarning: '請立即複製此連結。JCODE 不會保留其中的 fragment 金鑰。', | |
| active: '有效連結', | |
| revoke: '撤銷連結', | |
| revokeError: '無法撤銷此連結。', | |
| }, |
🤖 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 `@web/src/i18n/locales/zh-Hant.ts` around lines 1125 - 1144, Update the
user-visible product name references in shareDialog, specifically privacy and
keyWarning, from “JCode” to the file’s established “JCODE” casing. Leave the
surrounding translations unchanged.
Owner
Author
|
@jcode-cloud-app review |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
show_artifacttool to Web/Desktop full and plan modes only; CLI and ACP are intentionally unchanged.Companion Cloud API/viewer: cnjack/cloud#26
User impact
Artifacts produced by coding agents can be surfaced as durable, inspectable outputs in JCode Desktop/Web. The workflow requires no feature flag. Cloud sharing disappears when logged out and remains optional when authenticated.
Validation
go test ./...golangci-lint run --new-from-rev=HEAD: 0 issuesmake build-webkimi-for-coding / k3: created a Markdown file, calledshow_artifact, and rendered it automatically in the panelTest determinism
The existing provider-visibility fixture previously depended on a model fetched from the changing models.dev catalog. It now uses JCode's static Kimi provider and explicitly controls the default visibility under test, preserving the assertion while making CI reproducible.
Summary by CodeRabbit
New Features
Security