-
Notifications
You must be signed in to change notification settings - Fork 2
Ditto Web Chat – Milestone 2 Completion #82
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
…DittoChat into feature/web-rooms-chat
…DittoChat into feature/web-rooms-chat
…DittoChat into feature/web-subscriptions-notifications
…message & edit cancel on esc added & chat logout function added
…message & edit cancel on esc added & chat logout function added
.github/workflows/owasp-check.yml
Outdated
| --disableSwiftPackageManagerAnalyzer | ||
| --disableSwiftPackageResolvedAnalyzer |
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.
@ErikEverson potential integration in the future?
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.
Yeah
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.
I'd like input from @ErikEverson I think. Offhand I don't think a bunch of booleans is more scalable than a list of permissions, but I don't have a good grasp of our needs in this respect
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.
Yeah actually the most scaleable way we could do this is to have an enum of permissions and then an array of permissions could be used. This means that we get enum safety when we add a new permission and we also get the convenience of not having to add a new boolean and update every place in the code that it is used as well.
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.
@ErikEverson This probably needs to align with our colors, right? At least eventually
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.
Yes we will want this to align with the color stuff from forge as it is a really robust system. I think this is something to refactor when we get the color system finalized in Forge.
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.
Is there a reason we didn't go for an off the shelf solution for toasts like https://github.com/emilkowalski/sonner?
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.
We considered using an off-the-shelf toast library like Sonner, but for this project we wanted to minimize external dependencies and keep the UI layer purely Tailwind-driven. Since our toast requirements were fairly simple, we opted to implement a lightweight custom Toast component instead.
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.
I think it would be more maintainable long term to not re-invent things where possible. The unpacked size of something like sonner is pretty negligible for modern web applications (166kb for sonner specifically) and I'd argue that a headless UI component not only satisfies your desire to keep the UI layer purely Tailwind-driven but is more ideal since you still get complete control over styling and animations while the library handles the complex orchestration details as our needs grow (promises, ARIA, more queue management, etc).
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.
Added the sonner package and removed our custom toaster implementation.
We also introduced a new option that allows consumers to pass their own custom notification function through props. This gives integrators full control—if they want to handle notifications in their own way, they can simply provide a function that receives the notification details and display it however they prefer in the parent app.
If no custom function is provided, we fall back to using sonner by default to show notifications.
🟢 Test Coverage Report -
|
| Metric | Coverage | Status |
|---|---|---|
| 🟢 Lines | 93.78% | green |
| 🟢 Statements | 93.78% | green |
| 🟢 Functions | 95.83% | green |
| 🟢 Branches | 86.31% | green |
📊 View Detailed Coverage Report
ℹ️ Coverage Thresholds
- 🟢 Excellent (≥ 80%)
- 🟡 Good (60-79%)
- 🟠 Fair (40-59%)
- 🔴 Poor (< 40%)
🟢 Test Coverage Report -
|
| Metric | Coverage | Status |
|---|---|---|
| 🟢 Lines | 92.53% | green |
| 🟢 Statements | 92.53% | green |
| 🟢 Functions | 90.56% | green |
| 🟢 Branches | 88.8% | green |
📊 View Detailed Coverage Report
ℹ️ Coverage Thresholds
- 🟢 Excellent (≥ 80%)
- 🟡 Good (60-79%)
- 🟠 Fair (40-59%)
- 🔴 Poor (< 40%)
|
| Severity | Count |
|---|---|
| 🔴 Critical | 0 |
| 🟠 High | 0 |
| 🟡 Medium | 1 |
| 🔵 Low | 0 |
| Total | 1 |
📋 Vulnerability Details
- CVE-2025-64718 (MEDIUM) in
js-yaml:3.14.2- Description: js-yaml is a JavaScript YAML parser and dumper. In js-yaml 4.1.0 and below, it's possible for an attacker to modify the prototype of the result of a parsed yaml document via prototype pollution ( proto ). All users who parse untrusted yaml documents may be impacted. The problem is patched in js-yaml 4.1.1. Users can protect against this kind of attack on the server by using node --disable-proto=delete or deno (in Deno, pollution protection is on by default).
- CVSS: 5.3
ℹ️ How to fix vulnerabilities
- Update vulnerable dependencies to patched versions
- Run
npm audit fixornpm audit fix --force - Check for alternative packages if updates aren't available
- Review and update your
package.jsonandpackage-lock.json
|
| Severity | Count |
|---|---|
| 🔴 Critical | 0 |
| 🟠 High | 0 |
| 🟡 Medium | 1 |
| 🔵 Low | 0 |
| Total | 1 |
📋 Vulnerability Details
- CVE-2025-64718 (MEDIUM) in
js-yaml:3.14.2- Description: js-yaml is a JavaScript YAML parser and dumper. In js-yaml 4.1.0 and below, it's possible for an attacker to modify the prototype of the result of a parsed yaml document via prototype pollution ( proto ). All users who parse untrusted yaml documents may be impacted. The problem is patched in js-yaml 4.1.1. Users can protect against this kind of attack on the server by using node --disable-proto=delete or deno (in Deno, pollution protection is on by default).
- CVSS: 5.3
ℹ️ How to fix vulnerabilities
- Update vulnerable dependencies to patched versions
- Run
npm audit fixornpm audit fix --force - Check for alternative packages if updates aren't available
- Review and update your
package.jsonandpackage-lock.json
…e dittochatui readme
…itto/DittoChat into feature/web-chat-milestone2
ErikEverson
left a comment
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.
One thing to think about with RBAC
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.
Yeah actually the most scaleable way we could do this is to have an enum of permissions and then an array of permissions could be used. This means that we get enum safety when we add a new permission and we also get the convenience of not having to add a new boolean and update every place in the code that it is used as well.
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.
Yes we will want this to align with the color stuff from forge as it is a really robust system. I think this is something to refactor when we get the color system finalized in Forge.
Ditto Web Chat – Milestone 2 Completion
This PR completes Milestone 2 of the Ditto Web Chat project. It includes comprehensive unit test coverage, subscription management, notification system, reaction support, user mentions, dark theme implementation, profile picture rendering, security improvements and CI/CD pipelines.
Features Delivered
Issues Resolved