API Proposal #187
API Proposal #187
Conversation
|
Yeah, no, that CI status checks out considering I've been losing my mind over an issue with tsc |
|
Reviewing this in the browser is super inconvenient, I will submit a review once I get back home. |
KTrain5169
added
the
meta
| // /** Whether or not NeuroPilot is connected to the Neuro API. */ | ||
| // readonly connected: Omit<ConnectionTypes, ConnectionTypes.Failed>; | ||
| // /** Current connection info, assuming {@link APIWrapper.connected} returns {@link ConnectionTypes.Connected} */ | ||
| // readonly connectionInfo: ConnectionStatus | null; |
There was a problem hiding this comment.
Connection info was in case of debugging purposes, tbh idrc if we need it or not.
There was a problem hiding this comment.
I think all its properties can be obtained in other ways:
connectedcan be got withgetConnectionStatus().urlwould always be as configured in the settings. I assume the| nullpart of the type means that it would benullif not connected, which can be determined withgetConnectionStatus().- Any
errorduring connection would be logged to the output channel.
There was a problem hiding this comment.
True. That being said, do we need to expose the URL?
There was a problem hiding this comment.
I think you can query that from the settings, but I'll test that.
| * Deactivate the companion extension. | ||
| * Removes and unregisters all actions registered by this extension. | ||
| * After deactivation, the API methods will no longer work. | ||
| * @param message The message to display on deactivation. |
There was a problem hiding this comment.
I thought this would be a message that would be sent to Neuro on deactivation (alongside our own deactivation notice)?
There was a problem hiding this comment.
After thinking about this, it would probably make more sense to not provide a message here, since the companion can just send context to Neuro via sendContext(...), or display a vscode notification without relying on our API.
There was a problem hiding this comment.
is it possible to just not have a .ts file in here and instead just have a .d.ts file instead? publishing .ts files have some jank to them
There was a problem hiding this comment.
That is not possible because the enum has values defined, which either tsc or esbuild doesn't like. We could maybe try using a .js file instead if that's better.
There was a problem hiding this comment.
esbuild seems to like it now, wasnt complaining when I was trying
How the API works
The activation function returns a
NeuroPilotAPIWrapperobject. It has a single method (for now),registerCompanion, which allows a companion extension to register itself.registerCompaniongenerates a token for the companion and returns the actualExtensionAPIobject. The token is contained in the private_tokenproperty, and is used to validate "companion permissions" and filter actions in the registry. Currently, all companion permissions are always considered on, because no way to change them is implemented yet.I commented out some of the API functions which I deem unnecessary, but I am open to discussion on why they are necessary.
Safety
The API does not expose access to the action registry directly, so the filter cannot be circumvented. With knowledge about the implementation details, it is possible to extract the token from the API, but the token is not very useful by itself.