feat(ui): added support for Effect.ts playground links#3082
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Hello! Thank you for opening your first PR to npmx, @nikelborm! 🚀 Here’s what will happen next:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds Effect.ts playground support by recognising ChangesEffect.ts playground support
Sequence Diagram(s)sequenceDiagram
participant ReadmeContent
participant renderReadmeHtml
participant PLAYGROUND_PROVIDERS
participant PlaygroundsVue
ReadmeContent->>renderReadmeHtml: render Effect.ts playground link
renderReadmeHtml->>PLAYGROUND_PROVIDERS: match effect.website/play
PLAYGROUND_PROVIDERS-->>renderReadmeHtml: effect-ts-playground
renderReadmeHtml-->>PlaygroundsVue: provide playground link
PlaygroundsVue->>PlaygroundsVue: resolve Effect icon and colour classes
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@server/utils/readme.ts`:
- Around line 102-107: Update the route matching logic used by the readme
entries so the `effect-ts-playground` path `/play` matches only the exact
pathname or descendants beginning with `/play/`, not prefix-only near matches
such as `/playground` or `/player`. Add a regression test covering these
near-match paths while preserving valid `/play` and nested-route behavior.
In `@test/unit/server/utils/readme.spec.ts`:
- Around line 94-104: Update the test around renderReadmeHtml to assert that
each result.playgroundLinks contains an element before accessing index 0. Add
the presence or expected-length assertion for both result1 and result2, then
retain the existing provider assertions without non-null assertions.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 74d35806-342b-4f99-b7a9-8c889cc86885
📒 Files selected for processing (4)
app/components/Package/Playgrounds.vueserver/utils/readme.tstest/unit/server/utils/readme.spec.tsuno.theme.ts
| { | ||
| id: 'effect-ts-playground', | ||
| name: 'Effect.ts Playground', | ||
| domains: ['effect.website'], | ||
| paths: ['/play'], | ||
| icon: 'effect', |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Avoid prefix matching for the /play route.
The matcher uses pathname.startsWith(path), so this entry also classifies URLs such as /playground and /player as Effect.ts playgrounds. Match the exact route or a segment descendant (/play and /play/...) and add a near-match regression test.
🤖 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 `@server/utils/readme.ts` around lines 102 - 107, Update the route matching
logic used by the readme entries so the `effect-ts-playground` path `/play`
matches only the exact pathname or descendants beginning with `/play/`, not
prefix-only near matches such as `/playground` or `/player`. Add a regression
test covering these near-match paths while preserving valid `/play` and
nested-route behavior.
There was a problem hiding this comment.
The same feedback applies to the TypeScript case, which was used as a base for this addition. Fixing the prefix issue needs refactoring that is outside the scope of this PR.
There was a problem hiding this comment.
@nikelborm, agreed — this is pre-existing shared matcher behaviour, including the TypeScript provider, rather than a regression unique to the Effect.ts addition. I’ll leave it out of this PR. Would you like me to open a follow-up issue to tighten path matching across all providers?
| it('extracts Effect.ts playground links', async () => { | ||
| const markdown1 = `[Try it!](https://effect.website/play#3efe9f827b7d)` | ||
| const result1 = await renderReadmeHtml(markdown1, 'test-pkg') | ||
|
|
||
| expect(result1.playgroundLinks[0]!.provider).toBe('effect-ts-playground') | ||
|
|
||
| const markdown2 = `[Try it!](https://effect.website/play?code=Y29uc29sZS5sb2coKQ==)` | ||
| const result2 = await renderReadmeHtml(markdown2, 'test-pkg') | ||
|
|
||
| expect(result2.playgroundLinks[0]!.provider).toBe('effect-ts-playground') | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the link exists before indexing the array.
Both assertions use playgroundLinks[0]!; if extraction regresses, the test throws an unhelpful runtime error rather than reporting a failed assertion. Assert the expected length or element presence before reading index 0.
As per coding guidelines, array values accessed by index must be checked.
🤖 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 `@test/unit/server/utils/readme.spec.ts` around lines 94 - 104, Update the test
around renderReadmeHtml to assert that each result.playgroundLinks contains an
element before accessing index 0. Add the presence or expected-length assertion
for both result1 and result2, then retain the existing provider assertions
without non-null assertions.
Source: Coding guidelines
There was a problem hiding this comment.
The same feedback applies to the test, which was used as a base for this addition. Polishing the tests regarding this issue needs refactoring that is outside the scope of this PR.
There was a problem hiding this comment.
@nikelborm Understood. Keeping this focused on Effect.ts playground support is reasonable; the assertion-pattern cleanup can be handled consistently in a separate test refactor.
Would you like me to open a follow-up issue for it?
🐇💭
d90fea5 to
c6eca4f
Compare
|
Testing it on this page: Page on main: |
|
I would appreciate any hints on how to adapt it better to theme changes. Because I assume it needs refactoring of the underlying component, and it doesn't seem to be in the scope of this PR. Effect.ts's logo is monochrome and doesn't really have the accent color, as all other playground logos do. The brand guidelines explicitly state not to use any color other than black and white here: https://www.effect.website/brand-assets
|
1 participant
🔗 Linked issue
Resolves: #3081
🧭 Context
Effect.ts is an ecosystem of libraries that's gaining popularity (26 million downloads/week already for the main
effectpackage). It has a playground page where people can share specific environments prepared for their libraries.📚 Description
This PR makes packages with Effect.ts playground links in Readmes have them rendered in the right sidebar to encourage experimentation.