-
-
Notifications
You must be signed in to change notification settings - Fork 93
Add support for reference modifier #3133
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8e8184156
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| relative: getReferencePath("relative"), | ||
| remote, | ||
| remoteWithBranch, | ||
| remoteCanonical, | ||
| name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accept $canonical placeholder in reference snippets
The API docs for ReferenceSnippetDescriptor state that $canonical is a supported placeholder, but the substitution map only defines remoteCanonical (and subsituteSnippet throws on unknown variables). If a user writes a snippet using the documented $canonical placeholder, the reference modifier will throw “Unknown snippet variable: canonical” and fail. Either add a canonical alias in ReferenceStage or update the docs to match the implementation. See docs in packages/common/src/types/command/PartialTargetDescriptor.types.ts lines 423-426.
Useful? React with 👍 / 👎.
| const branchOrSha = isCanonical ? headSha : (defaultBranch ?? "main"); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid defaulting to 'main' when default branch unknown
getRemoteGitPath falls back to "main" whenever origin/HEAD can’t be resolved. In repos whose default branch isn’t main (eg master, trunk, or any custom branch), this generates reference links that 404, even though the repo is otherwise valid. This occurs whenever git symbolic-ref --short refs/remotes/origin/HEAD fails (eg no remote HEAD set or remotes not fetched). Consider falling back to headSha or returning undefined instead of hardcoding main.
Useful? React with 👍 / 👎.
just a local experiment I've been running. pushing so I can use from another computer and in case anyone wants to look. working well for me so far
can try locally by installing extension from this PR and using https://github.com/cursorless-dev/cursorless-talon-staging/tree/pr/3133 cursorless-talon branch