dev: add copilot-instructions.md #3172

marksvc · 2025-04-17T22:27:50Z

The instructions give context to Copilot to help it do a better job.

There is some duplication. And it's not super elegant. And some
instructions are not exactly verified. But so far it has been useful.

This change is

The instructions give context to Copilot to help it do a better job. There is some duplication. And it's not super elegant. And some instructions are not exactly verified. But so far it has been useful.

codecov · 2025-04-17T22:35:53Z

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
3870	1	3869	4

View the top 1 failed test(s) by shortest run time

.CheckingAudioPlayerComponent CheckingAudioPlayerComponent can play physical files

Stack Traces | 2.76s run time

Error: Expected '0:00' to be '0:01'.
    at <Jasmine>
    at UserContext.<anonymous> (.../checking/checking-audio-player/checking-audio-player.component.spec.ts:50:29)
    at Generator.next (<anonymous>)
    at asyncGeneratorStep (node_modules/@.../helpers/esm/asyncToGenerator.js:3:1)

To view more test analytics, go to the Test Analytics Dashboard
_{📋 Got 3 mins? Take this short survey to help us improve Test Analytics.}

Nateowami

Leaving some comments, but haven't fully reviewed everything. Overall looks good. I don't want to hold up merging over anything little.

I think it may be worth getting a review from someone else who has configured copilot this way. I'm guessing this instructional prompt improves over having no instructional prompt, but I can't speak for others who have done likewise, as I have nothing to compare to.

Reviewable status: 0 of 1 files reviewed, 8 unresolved discussions

.github/copilot-instructions.md line 13 at r1 (raw file):

- Data models must be defined in all 3 applications to stay in sync
- Frontend uses RealtimeService (ShareDB) for real-time data sync, which is defined at src/SIL.XForge.Scripture/ClientApp/src/xforge-common/realtime.service.ts
- Frontend works offline by default using local storage

localStorage !== IndexedDB

.github/copilot-instructions.md line 14 at r1 (raw file):

- Frontend uses RealtimeService (ShareDB) for real-time data sync, which is defined at src/SIL.XForge.Scripture/ClientApp/src/xforge-common/realtime.service.ts
- Frontend works offline by default using local storage
- Traditional REST endpoints are sometimes used

JSONRPC !== REST. I'm not sure there's anywhere we truly use REST. Maybe there are some exceptions?

.github/copilot-instructions.md line 15 at r1 (raw file):

- Frontend works offline by default using local storage
- Traditional REST endpoints are sometimes used
- Feature flags control functionality rollout

I would think you would state where they're placed to give it maximum chance to get it right (even just stating the service name)

.github/copilot-instructions.md line 16 at r1 (raw file):

- Traditional REST endpoints are sometimes used
- Feature flags control functionality rollout
- User permissions should be checked on both frontend and backend

...using the rights service whenever possible, rather than checking the user role

.github/copilot-instructions.md line 64 at r1 (raw file):

Please do not fail to add a comment to any classes that are created. All classes should have a comment.

Never rely on JavaScript's truthy or falsy. Instead, work with actual true, false, null, and undefined values, rather than relying on their interpretation as truthy or falsy.

I don't understand exactly what you intend by this. Do you mean when writing conditionals, do checks like if(thing != null) rather than if(thing)?

.github/copilot-instructions.md line 67 at r1 (raw file):

Specify types where possible, such as when declaring variables and arguments, and for function return types.
For example, do not write `const foo = 5`, but write `const foo: number = 5`.

I don't agree with this as a guideline. I think they should be specified only when they're not obvious, otherwise I don't see any value in them.

.github/copilot-instructions.md line 69 at r1 (raw file):

For example, do not write `const foo = 5`, but write `const foo: number = 5`.

If an async method is being called, but not awaited or returned, prefix the call with `void `.

This seems like it should be added as a lint rule.

.github/copilot-instructions.md line 77 at r1 (raw file):

Don't merely write code for the local context, but make changes that are good overall considering the architecture of the application and structure of the files and classes.

The frontend works offline, reading and writing data to and from a local store using src/SIL.XForge.Scripture/ClientApp/src/xforge-common/realtime.service.ts. This data is automatically kept up to date with the data on the server when the frontend has an Internet connection. Most of data reading and writing from the frontend should happen in this way, with occasional usage of web RPC, such as seen on the C# backend at src/SIL.XForge.Scripture/Controllers/SFProjectsRpcController.cs.

Instead of "a local store", it probably makes more sense to specify IndexedDB. And it seems like a major omission that it doesn't say "don't modify documents directly, instead submit operations to the documents"

marksvc

I am adding a commit "Revise" that accompanies my replies to Nathaniel.

I did some organizing and editing. I removed some items. This comes as a 3rd commit. I'm sorry that commit 3 may not be obvious to read from a diff from 1 or 2.

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @Nateowami)

.github/copilot-instructions.md line 13 at r1 (raw file):