Conversation
Preview deployments |
Host Test Results 1 files ± 0 1 suites ±0 3h 57m 9s ⏱️ + 2h 7m 28s For more details on these errors, see this check. Results for commit b87ce43. ± Comparison against base commit e021597. ♻️ This comment has been updated with latest results. |
![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: b87ce434ca
ℹ️ 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".
| await showCardCommand.execute({ | ||
| cardId: newFileUrl, |
There was a problem hiding this comment.
After the copy completes, this path always calls ShowCardCommand with newFileUrl, but ShowCardCommand reads via store.get(cardId) as a card by default (see packages/host/app/commands/show-card.ts), which does not work for normal file URLs like .txt/.png. In practice, the new “Copy to Workspace” flow can copy successfully and then immediately fail to open the copied file for typical file attachments.
Useful? React with 👍 / 👎.
| ); | ||
| } | ||
|
|
||
| let result = await this.cardService.copySource(sourceUrl, destinationUrl); |
There was a problem hiding this comment.
This command delegates directly to cardService.copySource without first validating the source exists, and copySource currently does a GET then unconditionally saves the response body to the destination (packages/host/app/services/card-service.ts). If sourceFileUrl is invalid or returns 404/500, the command can still write an error payload into a new file and report success, which silently creates corrupted copied files for malformed AI/tool inputs.
Useful? React with 👍 / 👎.
|
there's a fundamental issue here that we've discussed around copying before that has not been addressed--specifically how you deal with relationships. A card may have relative relationships to other cads in its own realm, or relationships to other cards in other private realms, as well as relationships to cards in public realms. same for modules. A card may import modules relatively in it's own realm, import modules in other private realms, or import modules in public realms. copying should be able to trace the graph of consumed dependencies and make a decision around what to pull in, and what to leave out. only in the most simplistic cases are you ever copying a single instance. many times though you need to consider the graph of dependencies (instances, file-defs, modules) that also need to be carried over. Failure to consider this means that you'll end up with an errored instance. Probably this is worth a discussion. |
There was a problem hiding this comment.
Pull request overview
Adds support for copying a file from one realm to another, and exposes it via a new “Copy to Workspace” menu item for File attachments shown in the AI Assistant.
Changes:
- Introduces
CopyFileToRealmCommand(host) plus command input/result types (base). - Registers/shims the command so it’s available from base code, and wires it into the FileDef menu for the
ai-assistantcontext. - Adds unit + integration tests covering the new menu item and command behavior (copying, conflict resolution, and permission error).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/host/tests/unit/file-def-menu-items-test.ts | Adds unit tests verifying “Copy to Workspace” presence/absence in AI-assistant context. |
| packages/host/tests/integration/commands/copy-file-to-realm-test.gts | Adds integration tests for copying files between realms, including conflict handling and permission errors. |
| packages/host/app/commands/index.ts | Registers/shims the new host command and adds it to HostCommandClasses. |
| packages/host/app/commands/copy-file-to-realm.ts | Implements the file-copy command, including conflict resolution. |
| packages/base/file-api.gts | Adds “Copy to Workspace” menu item for File attachments in AI Assistant context. |
| packages/base/command.gts | Adds CopyFileToRealmInput / CopyFileToRealmResult command schema types. |
Comments suppressed due to low confidence (5)
packages/host/tests/unit/file-def-menu-items-test.ts:72
- Test name says “with edit permission”, but the menu item is gated by
menuContextParams.canEditActiveRealm(notcanEdit). Renaming the test to mention “edit permission to active realm/workspace” would make the intent clearer and reduce confusion whencanEditandcanEditActiveRealmdiverge.
test('ai-assistant context with edit permission includes Copy to Workspace', function (assert: Assert) {
let file = new DummyFile(
'https://example.com/realm/file-5.txt',
) as unknown as FileDef;
packages/host/tests/unit/file-def-menu-items-test.ts:94
- Test name says “without edit permission”, but the behavior is controlled by
menuContextParams.canEditActiveRealm. Renaming this test to explicitly refer to active-realm/workspace edit permission would better match the implementation.
test('ai-assistant context without edit permission does not include Copy to Workspace', function (assert: Assert) {
let file = new DummyFile(
'https://example.com/realm/file-6.txt',
) as unknown as FileDef;
packages/host/app/commands/copy-file-to-realm.ts:40
targetRealmis used both forcanWrite()and as the base fornew URL(filename, targetRealm), but it’s not normalized to a trailing-slash realm URL. If a caller passeshttps://example.com/my-realm(no/),new URL()will resolve the destination against the parent path andrealm.canWrite()will likely return false because realm keys are stored with trailing slashes. NormalizetargetRealm(e.g., ensure it ends with/) before permission checks and URL construction.
let targetRealm =
input.targetRealm || this.realm.defaultWritableRealm?.path;
if (!targetRealm) {
throw new Error('No writable realm available to copy file to');
}
if (!this.realm.canWrite(targetRealm)) {
throw new Error(`Do not have write permissions to ${targetRealm}`);
}
packages/host/app/commands/copy-file-to-realm.ts:47
- The destination path is derived from only the basename (
pathname.split('/').pop()), which drops any nested directories from the source URL (e.g. copyingnested/deep.txtbecomesdeep.txt). This can cause collisions and unexpected placement. Consider preserving the full path relative to the source realm (or at least the full pathname minus the leading/) when constructingdestinationUrl.
let sourceUrl = new URL(input.sourceFileUrl);
let filename = decodeURIComponent(
sourceUrl.pathname.split('/').pop() ?? sourceUrl.pathname,
);
let destinationUrl = new URL(filename, targetRealm);
packages/host/app/commands/copy-file-to-realm.ts:77
fileExists()treats any non-404 response as “exists”. IfgetSource()returns an error status (401/403/5xx), conflict resolution will keep trying new names and may end up throwing the generic “Unable to find non-conflicting filename…” instead of surfacing the real error. Handle unexpected statuses explicitly (e.g., return true only for 200/406, false for 404, and throw otherwise).
private async fileExists(fileUrl: string): Promise<boolean> {
let getSourceResult = await this.cardService.getSource(new URL(fileUrl));
return getSourceResult.status !== 404;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
chris's feedback was that this is ok. we want the copy operation to be very primitive |
In this PR, I implemented a command to copy files from one realm to another. I also added a 'Copy File' menu item to files attached in the AI Assistant.