Add per-status share pages and sharing UX

[zzstoatzz]

No description provided.

[claude]

Code Review for PR #64: Add per-status share pages and sharing UX

✅ Overall Assessment

This PR implements a well-designed status sharing feature with thoughtful UX considerations. The implementation is clean and follows good coding practices. However, there are a few areas that need attention before merging.

---

🔍 Code Quality & Best Practices

Strengths:

• Clean separation of concerns with dedicated share endpoints and templates
• Good use of base64 URL-safe encoding for share tokens
• Progressive enhancement with fallback from native share → clipboard → window.open
• Proper caching of default labels in the JavaScript share functionality

Areas for Improvement:

• Consider extracting share token generation/decoding into a dedicated module for better testability
• The JavaScript share functionality could benefit from TypeScript for better type safety

---

🐛 Potential Bugs & Issues

1. Missing URL Validation - The share token decoding in status_share_page (src/api/status_read.rs:223-254) doesn't validate that the decoded URI is actually a valid ATProto URI format. Consider adding validation to ensure it starts with "at://"

2. Race Condition in JavaScript - The share button feedback timer (static/share.js:46-54) could have a race condition if clicked rapidly. The current implementation handles this correctly with clearTimeout, but consider adding debouncing.

3. Handle Resolution Fallback - When handle resolution fails (src/api/status_read.rs:281-293), the code silently continues with None. Consider logging at warn level since this affects user experience.

---

⚡ Performance Considerations

1. N+1 Query Issue - The feed endpoint (src/api/status_read.rs:176-200) updates handles in a loop after fetching statuses. Consider:

   • Batch fetching handles for all unique DIDs
   • Caching resolved handles with a TTL

2. Base64 Encoding Overhead - Every status in the feed gets a base64-encoded share token calculated (templates/feed.html:1094-1096). Consider:

   • Lazy loading share tokens only when needed
   • Pre-computing and storing share tokens in the database

3. DOM Manipulation - The infinite scroll implementation (templates/feed.html:1090-1153) creates HTML strings and uses innerHTML. Consider using template elements or DocumentFragment for better performance.

---

🔒 Security Concerns

1. XSS Risk with innerHTML - templates/feed.html:1105-1115 uses innerHTML with user-generated content. While the content appears to be escaped server-side, consider using safer DOM manipulation methods with createTextNode for user content.

2. Open Redirect Potential - The profile URLs (templates/feed.html:1093) fallback to bsky.app/profile/${status.author_did} without validation. Ensure DIDs are validated server-side.

3. Missing CSRF Protection - The hide-status admin endpoint doesn't appear to have CSRF protection. Consider adding CSRF tokens to state-changing operations.

---

🧪 Test Coverage

Missing Tests:

• No tests for the new status_share_page endpoint
• No tests for StatusFromDb::load_by_uri method
• No tests for share token generation/validation methods
• No JavaScript tests for the share functionality

Recommendations:

1. Add unit tests for:

   • share_token() encoding/decoding roundtrip
   • load_by_uri() with valid/invalid URIs
   • Share text generation methods

2. Add integration tests for:

   • Share page endpoint with valid/invalid tokens
   • Share page rendering with different status types

---

💡 Additional Suggestions

1. Accessibility: Add aria-live regions for the share button feedback messages
2. i18n: Consider preparing for internationalization of share text and labels
3. Analytics: Consider adding non-invasive analytics to track share button usage
4. Rate Limiting: Consider adding rate limiting to the share endpoint to prevent abuse
5. Documentation: Add documentation for the share URL format and token generation

---

✨ Positive Highlights

• Excellent progressive enhancement approach for sharing
• Clean and intuitive UI/UX for the share functionality
• Good error handling with user-friendly error pages
• Proper use of semantic HTML and ARIA labels

This is a solid implementation that adds valuable functionality to the platform. With the security and performance improvements addressed, this will be ready to merge.

[claude]

Code Review for PR #64: Add per-status share pages and sharing UX

Overall Assessment

This PR adds valuable sharing functionality with individual status pages and share buttons. The implementation is generally solid, but there are several areas that need attention before merging.

🟢 Strengths

• Clean separation of concerns: The share functionality is properly separated into its own template and JavaScript module
• Progressive enhancement: Falls back gracefully from native share API → clipboard → new window
• Good UX feedback: Visual feedback for share actions with automatic restoration
• Proper URL construction: Uses canonical URLs and handles path construction correctly
• Accessibility: Proper ARIA labels on share buttons

🔴 Critical Issues

1. SQL Query Pattern in load_by_uri

The function in src/db/models.rs:48 uses parameterized queries which is good, but ensure the URI is properly validated before database operations.

2. Missing Input Validation

The status_share_page handler doesn't validate the DID and rkey parameters before using them. Consider adding validation for:

• Valid DID format
• Valid rkey format (typically base32 encoded)

🟡 Code Quality Concerns

1. Repeated Handle Assignment Logic

The same handle assignment pattern is repeated 4+ times across the codebase. This pattern should be extracted into a helper function to reduce duplication and improve maintainability.

2. Complex Inline JavaScript Generation

The feed template generates complex JavaScript inline (templates/feed.html:1093-1127). This makes it harder to maintain and test. Consider moving share path calculation to a separate function.

3. Error Handling Inconsistency

Different error handling patterns are used throughout:

• Some use .unwrap_or(None)
• Others use explicit error logging
• Missing error boundaries in JavaScript

🔧 Performance Considerations

1. N+1 Query Pattern

When loading multiple statuses, handles are resolved individually for each status. Consider:

• Batch loading handles
• Caching resolved handles at the application level

2. JavaScript Performance

• Multiple DOM queries for same elements
• Event delegation could be improved for dynamically added share buttons

🛡️ Security Recommendations

1. Content Security Policy: Ensure CSP headers allow the inline scripts
2. Rate Limiting: Consider rate limiting the share endpoint to prevent abuse
3. URL Validation: Validate share URLs to prevent open redirect vulnerabilities

📝 Missing Test Coverage

No tests were found for the new functionality. Consider adding:

1. Unit tests for new model methods (share_title, share_description, etc.)
2. Integration tests for the share endpoint
3. JavaScript tests for share.js functionality
4. E2E tests for the share flow

🔍 Minor Issues

1. Naming consistency: Mix of share_path() method vs sharePath in JS
2. Dead code: title field in StatusShareTemplate is marked as #[allow(dead_code)]
3. Magic numbers: FEEDBACK_DURATION = 2200 should have a comment explaining the choice

✅ Recommendations Before Merge

1. High Priority:

   • Add input validation for DID and rkey parameters
   • Extract repeated handle assignment logic
   • Add at least basic unit tests for the new methods

2. Medium Priority:

   • Improve error handling consistency
   • Consider batch loading handles for performance
   • Add rate limiting to the share endpoint

3. Low Priority:

   • Clean up naming consistency
   • Add comments for magic numbers
   • Remove dead code markers

Summary

This is a well-thought-out feature that adds good value. With the critical security issues addressed and some refactoring for maintainability, it will be ready to merge. The share UX is particularly well done with its progressive enhancement approach.

[claude]

Code Review for PR #64: Add per-status share pages and sharing UX

✅ Strengths

Well-designed sharing feature: The implementation provides a comprehensive sharing experience with native share API support, fallback to clipboard copy, and graceful degradation to opening in new tab.

Good UX patterns:

• Visual feedback on share actions ("shared!", "copied!")
• Progressive enhancement with feature detection
• Accessible markup with proper ARIA labels

Clean separation of concerns: Share logic isolated in share.js, backend properly handles status resolution, templates are well-structured.

🔍 Areas for Improvement

1. Security Concerns

• XSS vulnerability in share text: The share_text() method in src/db/models.rs concatenates user-controlled text without sanitization. When this is displayed in HTML contexts or passed to share APIs, it could execute malicious scripts.

  // Line 307-310 in models.rs - text content needs escaping
  pub fn share_text(&self) -> String {
      self.share_caption()
          .map(|caption| format!("{} — {}", self.share_title(), caption))
          .unwrap_or_else(|| self.share_title())
  }

2. Performance Considerations

• N+1 query issue: In status_read.rs, handles are resolved individually for each status in loops (lines 60-64, 111-120, 195-199). Consider batch-fetching handles.

• Redundant DOM queries: In feed.html JavaScript, document.querySelector('.status-list') could be cached once rather than queried multiple times.

3. Code Quality Issues

• Missing input validation: The status_share_page handler doesn't validate DID format before processing, relying on error handling later.

• Incomplete error states: The share page template doesn't handle cases where handle resolution fails gracefully - it falls back to DIDs in URLs which aren't user-friendly.

• Duplicated logic: The share path generation logic appears in both Rust (share_path() method) and JavaScript (lines 1093-1108 in feed.html). Should be centralized.

4. Potential Bugs

• Race condition in feedback timer: In share.js, rapid clicks could cause label restoration issues:

  // Lines 36-44 - timer can be overwritten
  if (button.dataset.shareFeedbackTimer) {
      clearTimeout(Number(button.dataset.shareFeedbackTimer));
  }

• Memory leak risk: Event listeners attached via attachHideHandler aren't cleaned up when elements are removed from DOM.

5. Missing Test Coverage

No tests added for:

• New database methods (load_by_uri, share methods)
• Share endpoint (status_share_page)
• JavaScript share functionality
• Handle resolution in share context

📋 Recommendations

1. Sanitize user content before including in share text
2. Add input validation for DID/rkey parameters
3. Batch handle resolution to reduce database queries
4. Add unit tests for new model methods
5. Add integration tests for the share endpoint
6. Consider rate limiting the share endpoint to prevent abuse
7. Add OpenGraph meta tags to the share page template for better social media previews
8. Document the share URL format (/s/{did}/{rkey}) in code comments

✨ Overall Assessment

This is a solid implementation of a sharing feature with good UX considerations. The main concerns are around security (XSS prevention), performance (N+1 queries), and test coverage. With the recommended improvements, this would be production-ready.

The progressive enhancement approach and accessibility considerations are particularly well done. The feature adds real value to the status platform.

[claude]

Code Review for PR #64: Add per-status share pages and sharing UX

🎯 Summary

This PR adds a comprehensive sharing feature that creates dedicated share pages for individual statuses with proper social media integration and fallback mechanisms. The implementation is well-structured and follows good practices overall.

✅ Strengths

• Progressive enhancement: Excellent fallback chain from native share API → clipboard → opening link
• Accessible implementation: Proper ARIA labels and semantic HTML throughout
• Good separation of concerns: Clean split between backend routes, models, and templates
• User feedback: Visual feedback for share actions with proper timing and restoration

🚨 Potential Issues

1. Missing Input Validation

The status_share_page handler in src/api/status_read.rs:217 should validate the DID and rkey parameters:

• DIDs should match the expected format (did:plc:* or did:web:*)
• rkeys should be validated for expected format to prevent path traversal attacks
• Consider using a regex or dedicated validation function

2. Race Condition in Share Feedback

In static/share.js:47-64, multiple rapid clicks could create overlapping timers. Consider disabling the button during the feedback period to prevent user confusion.

3. Potential N+1 Query Issue

When loading multiple statuses with handles (src/api/status_read.rs:195), each status resolution makes a separate network call to resolve handles. Consider batching these lookups or implementing a cache.

💡 Performance Suggestions

1. Cache Handle Resolutions: The handle resolver makes network calls for each status. A simple LRU cache with TTL would significantly improve performance for frequently accessed handles.

2. Optimize Feed Loading: The infinite scroll implementation in templates/feed.html rebuilds DOM for each status. Consider using DocumentFragment for batch insertions to reduce reflows.

3. Bundle JavaScript: The share.js script is loaded separately. Consider bundling with other scripts to reduce HTTP requests, or use HTTP/2 push if available.

🔒 Security Considerations

1. XSS Prevention: Templates properly escape user content via Askama ✅

2. CORS/Framing: The share pages might be embedded. Consider adding X-Frame-Options or CSP frame-ancestors directive based on requirements.

3. Rate Limiting: The share endpoint could be used for scraping. Consider implementing rate limiting per IP/user.

🧪 Test Coverage Recommendations

Add test coverage for:

• StatusFromDb::load_by_uri with various URI formats and edge cases
• Share text generation methods (share_title, share_description) with empty/special characters
• JavaScript share functionality with different browser API availability
• Full integration test for the share flow

📝 Code Quality Observations

1. Error Handling Consistency: Mix of Result returns and unwrap_or usage. Consider standardizing error handling approach.

2. Magic Numbers: The feedback duration (2200ms) in share.js should be a named constant for maintainability.

3. Documentation: New public methods like share_title() and share_description() would benefit from rustdoc comments explaining their purpose and return values.

4. Template Duplication: Similar share button HTML appears in multiple templates. Consider extracting to a partial template.

✨ Overall Assessment

This is a well-implemented sharing feature with thoughtful UX considerations and proper fallback mechanisms. The code is clean and follows the existing codebase patterns well. The progressive enhancement approach for sharing is particularly commendable.

Recommendation: Approve with suggestions - The feature is ready for merge, but consider addressing the input validation and performance optimizations in a follow-up PR.

Great work on this feature! The attention to user experience and fallback handling shows careful consideration of real-world usage scenarios.