Resolve Linear updates with edits & length bug

[zuuring]

Notes

This resolves an issue where a Linear project update is reposted when an update is made to an existing update. If the request has updatedFrom, we can exclude this from being posted.

In addition, this caps the description text to 500 characters and excludes any text in the embed that is under a collapsable section with +++

[claude]

Claude finished @zuuring's task —— View job

---

Reviewing PR: Resolve Linear updates issues with Edited & length

Todo List:

• Read the changed file to understand the implementation
• Analyze code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Review security concerns
• Assess test coverage needs
• Provide comprehensive feedback

---

Code Review Feedback

I've analyzed the changes in discord-scripts/linear-integration.ts and overall the implementation looks solid and addresses the stated requirements effectively. Here are my findings:

✅ Strengths

1. Clear Problem Resolution: The changes properly address both issues mentioned in the PR:

   • Skip reposting edited updates by checking updatedFrom field (lines 70-73)
   • Cap description length to 500 characters with proper truncation (lines 78-80)
   • Remove collapsible sections using regex pattern (lines 58-62)

2. Good Error Handling: The implementation includes proper logging for skipped updates and maintains existing error handling patterns.

3. Code Style Compliance: The code follows the repository's style guide with no semicolons, proper TypeScript typing, and consistent formatting.

🔍 Potential Issues & Suggestions

Minor Logic Issue

Location: discord-scripts/linear-integration.ts:78-80

const trimmedDescription = description.length > 500 
  ? `${description.substring(0, 497)}...`
  : description

Issue: Using substring(0, 497) + "..." (3 chars) = 500 chars total, but the condition checks for > 500. This creates a potential off-by-one scenario where a 501-character string gets truncated to 500, but a 500-character string passes through unchanged.

Suggestion: For consistency, either:

• Use >= 500 in the condition, or
• Use substring(0, 500) for exact 500-char limit

Regex Robustness

Location: discord-scripts/linear-integration.ts:60-61

const regex = /\+\+\+[\s\S]*?\+\+\+/g

Observation: The regex correctly uses non-greedy matching (*?) and handles multiline content with [\s\S]. This should work well for the intended use case of removing collapsible sections.

Type Safety Enhancement

Location: discord-scripts/linear-integration.ts:39
The updatedFrom field is typed as Record<string, unknown> which is appropriate, but the code could benefit from a more specific check:

// Current: implicit truthy check
if (updatedFrom) {

// More explicit alternative:
if (updatedFrom && Object.keys(updatedFrom).length > 0) {

🚀 Performance Considerations

• Efficient String Operations: The regex replacement and substring operations are efficient for typical Linear update sizes
• Early Return Pattern: Properly exits early when updatedFrom is present, avoiding unnecessary processing

🔒 Security Assessment

• No Security Concerns: The changes don't introduce any security vulnerabilities
• Input Validation: Proper handling of potentially undefined data.body with fallback text
• Existing Security: Maintains the webhook signature verification and other security measures

🧪 Test Coverage Recommendations

The changes would benefit from unit tests covering:

1. Text Processing: Test hideTextBetweenMarkers() function with various inputs:

   • Text with single +++ markers
   • Text with multiple +++ sections
   • Text with nested or malformed markers
   • Empty strings and edge cases

2. Update Filtering: Test the updatedFrom logic:

   • Updates with updatedFrom present (should be skipped)
   • Updates without updatedFrom (should be processed)
   • Edge cases with empty updatedFrom objects

3. Length Truncation: Test description length handling:

   • Descriptions exactly 500 characters
   • Descriptions over 500 characters
   • Very short descriptions
   • Descriptions that become shorter after marker removal

📋 Overall Assessment

Code Quality: ⭐⭐⭐⭐ (4/5) - Well-structured, readable, and maintainable code that follows established patterns

Bug Risk: ⭐⭐⭐⭐⭐ (5/5) - Very low risk; changes are focused and defensive

Performance: ⭐⭐⭐⭐⭐ (5/5) - Efficient implementation with appropriate early exits

Security: ⭐⭐⭐⭐⭐ (5/5) - No security concerns introduced

✅ Recommendation

APPROVE - This is a solid implementation that effectively addresses the stated requirements. The minor suggestions above are optional improvements rather than blocking issues. The changes are focused, well-implemented, and maintain good code quality standards.

---

[claude]

Claude finished @zuuring's task —— View job

---

Reviewing PR: Resolve Linear updates issues with Edited & length

Todo List:

• Read the changed file to understand the implementation
• Analyze code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Review security concerns
• Assess test coverage needs
• Provide comprehensive feedback

---

Code Review Feedback

I've analyzed the changes in discord-scripts/linear-integration.ts and overall the implementation looks solid and addresses the stated requirements effectively. Here are my findings:

✅ Strengths

1. Clear Problem Resolution: The changes properly address both issues mentioned in the PR:

   • Skip reposting edited updates by checking updatedFrom field (lines 70-73)
   • Cap description length to 500 characters with proper truncation (lines 78-80)
   • Remove collapsible sections using regex pattern (lines 58-62)

2. Good Error Handling: The implementation includes proper logging for skipped updates and maintains existing error handling patterns.

3. Code Style Compliance: The code follows the repository's style guide with no semicolons, proper TypeScript typing, and consistent formatting.

🔍 Potential Issues & Suggestions

Minor Logic Issue

Location: discord-scripts/linear-integration.ts:78-80

const trimmedDescription = description.length > 500 
  ? `${description.substring(0, 500)}...`
  : description

Issue: Using substring(0, 500) + "..." (3 chars) = 503 chars total, but the condition checks for > 500. This creates a potential off-by-one scenario where a 501-character string gets truncated to 503, but a 500-character string passes through unchanged.

Suggestion: For consistency, either:

• Use >= 500 in the condition, or
• Use substring(0, 497) for exact 500-char limit

Regex Robustness

Location: discord-scripts/linear-integration.ts:60-61

const regex = /\+\+\+[\s\S]*?\+\+\+/g

Observation: The regex correctly uses non-greedy matching (*?) and handles multiline content with [\s\S]. This should work well for the intended use case of removing collapsible sections.

Type Safety Enhancement

Location: discord-scripts/linear-integration.ts:39
The updatedFrom field is typed as Record<string, unknown> which is appropriate, but the code could benefit from a more specific check:

// Current: implicit truthy check
if (updatedFrom) {

// More explicit alternative:
if (updatedFrom && Object.keys(updatedFrom).length > 0) {

🚀 Performance Considerations

• Efficient String Operations: The regex replacement and substring operations are efficient for typical Linear update sizes
• Early Return Pattern: Properly exits early when updatedFrom is present, avoiding unnecessary processing

🔒 Security Assessment

• No Security Concerns: The changes don't introduce any security vulnerabilities
• Input Validation: Proper handling of potentially undefined data.body with fallback text
• Existing Security: Maintains the webhook signature verification and other security measures

🧪 Test Coverage Recommendations

The changes would benefit from unit tests covering:

1. Text Processing: Test hideTextBetweenMarkers() function with various inputs:

   • Text with single +++ markers
   • Text with multiple +++ sections
   • Text with nested or malformed markers
   • Empty strings and edge cases

2. Update Filtering: Test the updatedFrom logic:

   • Updates with updatedFrom present (should be skipped)
   • Updates without updatedFrom (should be processed)
   • Edge cases with empty updatedFrom objects

3. Length Truncation: Test description length handling:

   • Descriptions exactly 500 characters
   • Descriptions over 500 characters
   • Very short descriptions
   • Descriptions that become shorter after marker removal

📋 Overall Assessment

Code Quality: ⭐⭐⭐⭐ (4/5) - Well-structured, readable, and maintainable code that follows established patterns

Bug Risk: ⭐⭐⭐⭐⭐ (5/5) - Very low risk; changes are focused and defensive

Performance: ⭐⭐⭐⭐⭐ (5/5) - Efficient implementation with appropriate early exits

Security: ⭐⭐⭐⭐⭐ (5/5) - No security concerns introduced

✅ Recommendation

APPROVE - This is a solid implementation that effectively addresses the stated requirements. The minor suggestions above are optional improvements rather than blocking issues. The changes are focused, well-implemented, and maintain good code quality standards.

---

[Shadowfiend]

Is the outcome here that updates aren't posted, or that existing embeds are edited?

[zuuring]

Is the outcome here that updates aren't posted, or that existing embeds are edited?

At the moment it just ignores edited projectupdate events hitting the webhook, but we could also edit the embeds. Just would need to add some additional logic here to match it. wdyt?

[Shadowfiend]

I think let's not do it yet. That kind of updating logic is exactly where I want us to move to a worker backed by a proper db, which could even be D2 for this use case.