Repository navigation
Rename diff-viewer package to webviews and pin TanStack Router - #5426
lawrencecchen wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
|
Complex PR? Review this PR in Change Stack to move by importance, not file order. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR refactors the diff-viewer package to webviews across build infrastructure, CI, and asset paths, while integrating ChangesWebviews Rename and TanStack Router Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 18 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (18 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
![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: 716ca8fc89
ℹ️ 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".
| const compromisedVersions = new Map([ | ||
| ["@tanstack/history", new Set(["1.161.9", "1.161.12"])], | ||
| ["@tanstack/react-router", new Set(["1.169.5", "1.169.8"])], | ||
| ["@tanstack/router-core", new Set(["1.169.5", "1.169.8"])], |
There was a problem hiding this comment.
When another GHSA-g7cv-rxg3-hmpx-affected TanStack package is added to this lockfile, this guard will still pass for known-malicious versions because the blocklist only includes three packages; the advisory covers many more affected @tanstack/* packages (for example @tanstack/react-router-devtools 1.166.16/1.166.19). Since this script is now the build-time security gate for TanStack lockfile drift, leaving the rest of the affected package/version pairs out makes the verifier give a false sense of coverage for future TanStack additions.
Useful? React with 👍 / 👎.
716ca8f to
b202f5b
Compare
lawrencecchen
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b202f5b. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b202f5bda4
ℹ️ 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".
|
|
||
| function parseTanstackLockEntries(text) { | ||
| const entries = new Map(); | ||
| const linePattern = /^\s+"(@tanstack\/[^"]+)": \["@tanstack\/[^@"]+@([^"]+)",.*"(sha512-[^"]+)"\],?$/gm; |
There was a problem hiding this comment.
When Bun has to keep a second copy of a package for a specific dependency, this lockfile format can use prefixed keys like parent/package while the package spec inside the array still names the real package; there are already entries of that form elsewhere in webviews/bun.lock (for example bundled/nested packages). This regex only matches keys that start exactly with @tanstack/, so a future dependency that pulls a nested compromised @tanstack/history or @tanstack/router-core would not be included in lockEntries and the GHSA blocklist loop would pass even though the bad package is present. Parse the package name from the array spec (or otherwise scan all package records) so nested TanStack copies are checked too.
Useful? React with 👍 / 👎.
Greptile SummaryRenames the embedded React app source and asset bundle from
Confidence Score: 4/5The rename is internally consistent across scripts, CI, and Swift production code, but the Swift test suite was not updated to match the new asset-directory names. The Swift production code correctly looks for cmuxTests/CMUXOpenCommandTests.swift — the test helper and multiple assertions reference the old asset-directory names and need to be updated to match the renamed paths in CLI/cmux_open.swift. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[bun run build] --> B[verify:tanstack-router]
B --> C{package.json pin == 1.170.11?}
C -- No --> FAIL1[exit 1]
C -- Yes --> D{bun.lock has all 5 TanStack entries with expected hashes?}
D -- No / hash mismatch --> FAIL2[exit 1]
D -- Yes --> E{Any @tanstack/* entry matches compromised version set?}
E -- Yes --> FAIL3[exit 1 GHSA-g7cv-rxg3-hmpx]
E -- No --> F[bun run typecheck]
F --> G[vite build]
G --> H[Resources/markdown-viewer/webviews-app/main.mjs]
H --> I[cmux_open.swift diffViewerBundledAppAssetDirectory looks for webviews-app]
I --> J[copies as cmux-webviews-app in viewer assets dir]
Reviews (2): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function parseTanstackLockEntries(text) { | ||
| const entries = new Map(); | ||
| const linePattern = /^\s+"(@tanstack\/[^"]+)": \["@tanstack\/[^@"]+@([^"]+)",.*"(sha512-[^"]+)"\],?$/gm; | ||
| for (const match of text.matchAll(linePattern)) { | ||
| entries.set(match[1], { | ||
| version: match[2], | ||
| integrity: match[3], | ||
| }); | ||
| } | ||
| return entries; | ||
| } |
There was a problem hiding this comment.
parseTanstackLockEntries accumulates results into a Map keyed by package name (e.g. @tanstack/react-router). If bun.lock ever contains two lines with the same @tanstack/ package name — for instance via an aliased install or a future nested-workspace setup — the Map silently overwrites the first entry with the second. The compromised-version sweep on lines 55-60 then only sees the last occurrence, so a compromised version that happens to appear earlier in the file would be invisible to the check. For a security-critical verifier, collecting all matches into an array and iterating all of them (or failing fast on any duplicate key) would close this gap without changing normal-case behavior.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| "dependencies": { | ||
| "@pierre/diffs": "1.2.7", | ||
| "@pierre/trees": "1.0.0-beta.4", | ||
| "@tanstack/react-router": "1.170.11", |
There was a problem hiding this comment.
@tanstack/react-router is declared but not imported anywhere in source
@tanstack/react-router is listed under dependencies, but no file in webviews/src/ imports it. Is this intentional pre-emptive pinning ahead of a follow-up PR that adds router usage, or was an import accidentally left out? If it's proactive pinning only, a comment in package.json (or the PR description) clarifying this would help future maintainers understand why an apparently unused dependency is present and security-verified.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
1-13:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit least-privilege GitHub token permissions.
Line 1 starts a workflow without a
permissionsblock, so jobs run with default token scopes. Please pin minimal permissions at workflow level (and elevate per job only where required) to avoid accidental over-privileged CI tokens.Suggested hardening
name: CI +permissions: + contents: read🤖 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 @.github/workflows/ci.yml around lines 1 - 13, Add a workflow-level permissions block to the CI workflow to enforce least-privilege for GITHUB_TOKEN (e.g., set read-only for repo contents and other minimal scopes the majority of jobs need) and remove relying on default token scopes; then, for any job that requires broader access, add a job-level permissions override (permissions:) inside that job to explicitly grant only the additional scopes it needs. Update the top-level of the "CI" workflow (the workflow root) to include the minimal permissions and adjust specific jobs that need push/write or id-token to explicitly elevate there.
🤖 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.
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 1-13: Add a workflow-level permissions block to the CI workflow to
enforce least-privilege for GITHUB_TOKEN (e.g., set read-only for repo contents
and other minimal scopes the majority of jobs need) and remove relying on
default token scopes; then, for any job that requires broader access, add a
job-level permissions override (permissions:) inside that job to explicitly
grant only the additional scopes it needs. Update the top-level of the "CI"
workflow (the workflow root) to include the minimal permissions and adjust
specific jobs that need push/write or id-token to explicitly elevate there.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f14b07a7-cc2f-4ae4-b442-3ca69b56f186
⛔ Files ignored due to path filters (1)
webviews/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (36)
.github/workflows/ci.ymlCLI/cmux_open.swiftResources/markdown-viewer/webviews-app/main.mjsscripts/build-webviews-app.shscripts/check-webviews-react-compiler.mjswebviews/README.mdwebviews/index.htmlwebviews/package.jsonwebviews/scripts/verify-tanstack-router-security.mjswebviews/src/App.tsxwebviews/src/actions.tswebviews/src/appearance.tswebviews/src/diff-stream.tswebviews/src/file-tree-refresh.tswebviews/src/global.d.tswebviews/src/icons.tsxwebviews/src/labels.tswebviews/src/main.tsxwebviews/src/pierre-options.tswebviews/src/status.tswebviews/src/styles.csswebviews/src/types.tswebviews/src/worker-pool.tswebviews/test/actions.test.tswebviews/test/app.test.tsxwebviews/test/appearance.test.tswebviews/test/diff-stream.test.tswebviews/test/file-tree-refresh.test.tswebviews/test/labels.test.tswebviews/test/pierre-options.test.tswebviews/test/pierre-tree-bundle.test.tswebviews/test/status.test.tswebviews/test/styles.test.tswebviews/test/worker-pool.test.tswebviews/tsconfig.jsonwebviews/vite.config.mjs
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5b4a2568e
ℹ️ 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".
| let appDirectory = sourceDirectory | ||
| .deletingLastPathComponent() | ||
| .appendingPathComponent("diff-viewer-app", isDirectory: true) | ||
| .appendingPathComponent("webviews-app", isDirectory: true) |
There was a problem hiding this comment.
When the Swift CLI tests build their temporary fixture resources, writeTestDiffViewerAssets still creates markdown-viewer/diff-viewer-app, and several CMUXOpenCommandTests assertions still look for cmux-diff-viewer-app/main.mjs; with this lookup now requiring markdown-viewer/webviews-app, those tests either throw Bundled cmux diff viewer app assets not found before generating the viewer or fail the expected asset URL checks. Update the test fixtures/assertions in the same rename so the CLI test suite can pass.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 @.github/workflows/ci.yml:
- Around line 289-296: The CI change introducing an isolated DerivedData path
(the "Prepare isolated DerivedData" step in .github/workflows/ci.yml that
defines DERIVED_DATA_PATH and exports CMUX_DERIVED_DATA_PATH) is unrelated to
the webviews rename and TanStack Router pin, so revert or remove that block from
this PR and move it into a separate commit/PR dedicated to CI/infrastructure
changes; specifically, remove the DERIVED_DATA_PATH creation and the echo
"CMUX_DERIVED_DATA_PATH=..." line from this workflow here, then create a new PR
that adds the Prepare isolated DerivedData step (using RUNNER_TEMP,
GITHUB_RUN_ID, GITHUB_RUN_ATTEMPT) as a standalone change with its own
description and tests so infrastructure changes are traceable and rollbackable
independently.
- Around line 10-12: The workflow currently grants broad write scope via the
permissions setting "actions: write"; since the job only uses
actions/upload-artifact, actions/download-artifact and the
concurrency.cancel-in-progress feature, remove "actions: write" from the
permissions block (leaving actions at the default or setting it to "read") or
add a clear comment/PR description justifying why "actions: write" is required;
update the permissions block in .github/workflows/ci.yml and ensure any
justification references the specific need for actions write scope if you keep
it.
In `@CLI/cmux_open.swift`:
- Around line 942-945: The guard currently accepts paths based only on
FileManager.default.isExecutableFile(atPath: candidate.path) which can be true
for directories; update the check in the routine that returns
canonicalFileURL(candidate) so directory-valued candidates are rejected first.
Use FileManager.default.fileExists(atPath:isDirectory:) (or URL
resourceValues/.isDirectory) to ensure the candidate isDirectory == false before
calling isExecutableFile, and only return canonicalFileURL(candidate) when both
"not a directory" and "isExecutableFile" are satisfied.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 560e1135-223e-4962-9251-8f22ddada793
📒 Files selected for processing (2)
.github/workflows/ci.ymlCLI/cmux_open.swift
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🤖 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 @.github/workflows/ci.yml:
- Around line 289-296: The CI change introducing an isolated DerivedData path
(the "Prepare isolated DerivedData" step in .github/workflows/ci.yml that
defines DERIVED_DATA_PATH and exports CMUX_DERIVED_DATA_PATH) is unrelated to
the webviews rename and TanStack Router pin, so revert or remove that block from
this PR and move it into a separate commit/PR dedicated to CI/infrastructure
changes; specifically, remove the DERIVED_DATA_PATH creation and the echo
"CMUX_DERIVED_DATA_PATH=..." line from this workflow here, then create a new PR
that adds the Prepare isolated DerivedData step (using RUNNER_TEMP,
GITHUB_RUN_ID, GITHUB_RUN_ATTEMPT) as a standalone change with its own
description and tests so infrastructure changes are traceable and rollbackable
independently.
- Around line 10-12: The workflow currently grants broad write scope via the
permissions setting "actions: write"; since the job only uses
actions/upload-artifact, actions/download-artifact and the
concurrency.cancel-in-progress feature, remove "actions: write" from the
permissions block (leaving actions at the default or setting it to "read") or
add a clear comment/PR description justifying why "actions: write" is required;
update the permissions block in .github/workflows/ci.yml and ensure any
justification references the specific need for actions write scope if you keep
it.
In `@CLI/cmux_open.swift`:
- Around line 942-945: The guard currently accepts paths based only on
FileManager.default.isExecutableFile(atPath: candidate.path) which can be true
for directories; update the check in the routine that returns
canonicalFileURL(candidate) so directory-valued candidates are rejected first.
Use FileManager.default.fileExists(atPath:isDirectory:) (or URL
resourceValues/.isDirectory) to ensure the candidate isDirectory == false before
calling isExecutableFile, and only return canonicalFileURL(candidate) when both
"not a directory" and "isExecutableFile" are satisfied.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 560e1135-223e-4962-9251-8f22ddada793
📒 Files selected for processing (2)
.github/workflows/ci.ymlCLI/cmux_open.swift
🛑 Comments failed to post (3)
.github/workflows/ci.yml (2)
10-12:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Search for workflow steps that might need actions: write permission rg -n -C3 'actions/(cancel-workflow|upload-artifact|download-artifact)' .github/workflows/ci.ymlRepository: manaflow-ai/cmux
Length of output: 748
🏁 Script executed:
#!/bin/bash set -euo pipefail f=".github/workflows/ci.yml" echo "== Find cancel-in-progress usage ==" rg -n "cancel-in-progress" "$f" || true echo echo "== List uses: actions/* in this workflow ==" rg -n "uses:\s*actions/" "$f" || true echo echo "== Show top-level permissions block ==" rg -n "^\s*permissions:|^\s*contents:|^\s*actions:" -n "$f" || trueRepository: manaflow-ai/cmux
Length of output: 2489
🌐 Web query:
Do GitHub Actions upload/download artifacts requirepermissions: actions: write? What permissions are required foractions/upload-artifactandactions/download-artifact?💡 Result:
GitHub Actions
actions/upload-artifactandactions/download-artifactdo not require explicitpermissions: actions: writeto perform standard operations within the current workflow run [1][2]. For standard usage—uploading and downloading artifacts within the same repository and the current workflow run—the actions utilize internal authentication mechanisms (often involving scoped access signatures) that do not require elevated repository permissions [1][2]. The specific permissions required depend on the scope of the action: 1. Standard Operations (Current Run): - No explicitpermissionsblock grantingactions: writeoractions: readis required for the action to function within the current workflow run [1][2]. 2. Cross-Repository or Cross-Run Operations: - When you need to download artifacts from a different repository or a different workflow run, you must provide agithub-tokenwith at leastactions: readpermissions for the target repository [3][4][2]. - If you need to perform administrative tasks, such as deleting artifacts from other repositories or runs, you would require a token withactions: writepermissions [5]. In summary, you do not needactions: writefor basic uploading and downloading, and you only need specificactionspermissions when extending the scope of these actions beyond the current workflow execution [5][2].Citations:
- 1: actions/upload-artifact#197
- 2: https://github.blog/news-insights/product-news/get-started-with-v4-of-github-actions-artifacts/
- 3: https://github.com/actions/download-artifact
- 4: https://github.com/actions/download-artifact/blob/main/README.md
- 5: https://github.com/actions/toolkit/blob/main/packages/artifact/README.md
🏁 Script executed:
#!/bin/bash set -euo pipefail f=".github/workflows/ci.yml" echo "== Any other explicit permissions blocks in jobs? ==" rg -n "^\s*permissions:" "$f" || true echo echo "== Any github-token / token inputs passed to actions? ==" rg -n "github-token:|token: *\$\{\{.*\}\}" "$f" || true echo echo "== Search for artifact deletion / other actions needing elevated permissions ==" rg -n "delete-artifact|cancel-workflow|workflow.*cancel|octokit|api\.github|rest\.|actions: write" "$f" || trueRepository: manaflow-ai/cmux
Length of output: 288
🌐 Web query:
Does GitHub Actions concurrencycancel-in-progress: truerequirepermissions: actions: write(or anypermissionsoverride), or is it handled by the runner/scheduler automatically?💡 Result:
You do not need to grant
permissions: actions: write(or any explicitpermissionsoverride) specifically forconcurrency.cancel-in-progress: true. GitHub’s documentation describescancel-in-progress: trueas a native scheduler/server behavior for canceling in-progress runs within the same concurrency group; it does not state any requiredpermissionsfor this mechanism [1][2]. In contrast, marketplace actions that cancel workflow runs via the Actions API explicitly document requiringactions: write(e.g.,cancel-workflow-action/cancel-queued-runs), because those implementations call the API rather than relying on the native concurrency cancel feature [3][4]. So:cancel-in-progressis handled automatically by GitHub Actions’ concurrency controls;permissions: actions: writeis only relevant if you’re using an external action/tool that cancels runs through the REST API (or similar), not for the built-inconcurrencykey itself [1][2].Citations:
- 1: https://docs.github.com/en/enterprise-cloud@latest/actions/using-jobs/using-concurrency
- 2: https://docs.github.com/actions/using-workflows/workflow-syntax-for-github-actions
- 3: https://github.com/marketplace/actions/cancel-queued-runs
- 4: https://github.com/marketplace/actions/cancel-workflow-action
🏁 Script executed:
#!/bin/bash f=".github/workflows/ci.yml" sed -n '1,60p' "$f" | cat -nRepository: manaflow-ai/cmux
Length of output: 2256
🏁 Script executed:
#!/bin/bash f=".github/workflows/ci.yml" echo "== Search for any uses that likely need elevated actions: write ==" rg -n "delete-artifact|cancel-workflow|cancel-queued|github\.api|octokit|rest\." "$f" || true echo echo "== Search for any explicit workflow/runner cancellation helpers ==" rg -n "cancel-in-progress|cancel-workflow|cancel-queued" "$f" || true echo echo "== Search for github-script / actions/github-script usage ==" rg -n "actions/github-script|github-script" "$f" || true echo echo "== Search for API delete / artifacts delete commands ==" rg -n "artifact.*delete|delete.*artifact|gh api.*artifact|rest.*actions" "$f" || trueRepository: manaflow-ai/cmux
Length of output: 382
Restrict/justify
permissions.actions: writein CI workflow
.github/workflows/ci.ymlsetspermissions: actions: write(lines 10-12), but the workflow only usesactions/upload-artifact/actions/download-artifactand nativeconcurrency.cancel-in-progress; there are no workflow/API steps found that would require elevatedactions: write(e.g., no REST/Octokit calls, artifact deletion, or external cancellation actions). Removeactions: writeor add a clear PR justification for why it’s needed at the workflow level.🧰 Tools
🪛 zizmor (1.25.2)
[error] 12-12: overly broad permissions (excessive-permissions): actions: write is overly broad at the workflow level
(excessive-permissions)
[warning] 12-12: permissions without explanatory comments (undocumented-permissions): needs an explanatory comment
(undocumented-permissions)
🤖 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 @.github/workflows/ci.yml around lines 10 - 12, The workflow currently grants broad write scope via the permissions setting "actions: write"; since the job only uses actions/upload-artifact, actions/download-artifact and the concurrency.cancel-in-progress feature, remove "actions: write" from the permissions block (leaving actions at the default or setting it to "read") or add a clear comment/PR description justifying why "actions: write" is required; update the permissions block in .github/workflows/ci.yml and ensure any justification references the specific need for actions write scope if you keep it.
289-296: 🧹 Nitpick | 🔵 Trivial | 💤 Low value
DerivedData isolation is a separate concern from webviews rename.
The addition of isolated DerivedData paths (with
RUNNER_TEMP,GITHUB_RUN_ID, andGITHUB_RUN_ATTEMPT) is a solid CI reliability improvement that prevents cache pollution between jobs and retry attempts. However, this change is orthogonal to the PR's stated objectives (renaming diff-viewer to webviews and pinning TanStack Router).While not harmful, bundling infrastructure improvements with feature changes makes it harder to trace regressions and complicates rollback. Consider separating infrastructure changes into dedicated PRs in the future.
Also applies to: 521-526, 872-877
🤖 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 @.github/workflows/ci.yml around lines 289 - 296, The CI change introducing an isolated DerivedData path (the "Prepare isolated DerivedData" step in .github/workflows/ci.yml that defines DERIVED_DATA_PATH and exports CMUX_DERIVED_DATA_PATH) is unrelated to the webviews rename and TanStack Router pin, so revert or remove that block from this PR and move it into a separate commit/PR dedicated to CI/infrastructure changes; specifically, remove the DERIVED_DATA_PATH creation and the echo "CMUX_DERIVED_DATA_PATH=..." line from this workflow here, then create a new PR that adds the Prepare isolated DerivedData step (using RUNNER_TEMP, GITHUB_RUN_ID, GITHUB_RUN_ATTEMPT) as a standalone change with its own description and tests so infrastructure changes are traceable and rollbackable independently.CLI/cmux_open.swift (1)
942-945:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject directory-valued runtime executable candidates before accepting tag runtime.
At Line 942,
isExecutableFile(atPath:)alone can treat a directory as executable. If that happens, the tagged runtime path is accepted and later launch fails instead of falling back cleanly.Suggested fix
- guard FileManager.default.isExecutableFile(atPath: candidate.path) else { + var isDirectory: ObjCBool = false + guard FileManager.default.fileExists(atPath: candidate.path, isDirectory: &isDirectory), + !isDirectory.boolValue, + FileManager.default.isExecutableFile(atPath: candidate.path) else { return nil }Based on learnings: user-provided/custom executable paths in this repo should verify
isDirectory == falsebeforeisExecutableFileso invalid directory candidates are rejected and safe fallback remains possible.🤖 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 `@CLI/cmux_open.swift` around lines 942 - 945, The guard currently accepts paths based only on FileManager.default.isExecutableFile(atPath: candidate.path) which can be true for directories; update the check in the routine that returns canonicalFileURL(candidate) so directory-valued candidates are rejected first. Use FileManager.default.fileExists(atPath:isDirectory:) (or URL resourceValues/.isDirectory) to ensure the candidate isDirectory == false before calling isExecutableFile, and only return canonicalFileURL(candidate) when both "not a directory" and "isExecutableFile" are satisfied.Source: Learnings
CodeRabbit now posts non-blocking comment reviews (request_changes_workflow=false, #5538).
|
Superseded: main already has the diff-viewer→webviews rename, the pinned @tanstack/react-router 1.170.11, and the verify:tanstack-router script (landed via the webviews split work, e.g. #5613). Closing as part of the editor-gui board reconcile. |
This branch was successfully deployed
1 participant
Summary
Verification
Security references:
Note
Medium Risk
Bundled asset path renames must stay aligned with Swift copy logic or the in-app diff viewer can fail to load; the new dependency adds supply-chain surface area mitigated by exact pins and integrity checks.
Overview
Rebrands the embedded React bundle from diff-viewer to webviews so the same package can host multiple in-app webviews (the diff UI remains the first consumer).
Naming and packaging:
@cmux/diff-viewerbecomes@cmux/webviews; source lives underwebviews/; Vite output and committed assets move toResources/markdown-viewer/webviews-appwith envCMUX_WEBVIEWS_OUT_DIR. CI’sreact-apps-checkjob and scriptsbuild-webviews-app.sh/check-webviews-react-compiler.mjsreplace the diff-viewer equivalents.macOS integration: Swift copies bundled app assets from
webviews-appand exposes them ascmux-webviews-appwhen preparing the diff viewer runtime.TanStack Router: Adds exact-pinned
@tanstack/react-router@1.170.11and runsverify-tanstack-router-security.mjsbefore build to enforce lockfile versions/integrity and block known compromised releases (GHSA-g7cv-rxg3-hmpx).Reviewed by Cursor Bugbot for commit e5b4a25. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Documentation
Chores
Dependencies
Tests