fix(apply): add support for Spotify 1.2.78+
#3629
Conversation
- Add new regex patterns for async React lazy loading in apply.go - Add FindSymbolWithPattern helper to return matched pattern for reuse - Fix Image Snackbar preprocessing to only match in assignment context - Add defer attribute to Spicetify.Config script block
WalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/utils/utils.go (1)
316-332: LGTM! Consider optional DRY refactor.The function is well-implemented and follows the existing codebase patterns. Returning both the matched symbols and the pattern enables more precise subsequent replacements in
apply.go.Minor DRY opportunity:
FindSymbolandFindSymbolWithPatternshare nearly identical logic. You could optionally refactor to haveFindSymbolcallFindSymbolWithPatterninternally:func FindSymbol(debugInfo, content string, clues []string) []string { - for _, v := range clues { - re := regexp.MustCompile(v) - found := re.FindStringSubmatch(content) - if found != nil { - return found[1:] - } - } - - if len(debugInfo) > 0 { - PrintError("Cannot find symbol for " + debugInfo) - } - - return nil + symbols, _ := FindSymbolWithPattern(debugInfo, content, clues) + return symbols }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/apply/apply.go(3 hunks)src/preprocess/preprocess.go(1 hunks)src/utils/utils.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/utils/utils.go (1)
src/utils/print.go (1)
PrintError(88-90)
src/apply/apply.go (1)
src/utils/utils.go (2)
FindSymbolWithPattern(318-332)ReplaceOnce(175-188)
🔇 Additional comments (5)
src/preprocess/preprocess.go (1)
949-956: LGTM! Good defensive pattern matching.The changes properly address the Image Snackbar patch:
- Capturing
=in group 1 ensures we only match in assignment contexts, preventing false positives- Adding
Once: trueprevents double-patching if the pattern appears multiple timessrc/apply/apply.go (4)
161-170: LGTM! Correct use ofdeferfor inline script.Adding
deferensures the Spicetify.Config assignment runs after the DOM is parsed, which is appropriate for configuration scripts.
263-271: Good pattern expansion for Spotify 1.2.78+ support.The three React lazy loading patterns correctly cover:
- Synchronous lazy loading (existing versions)
- Async lazy loading with
await(1.2.78+)- Async
Promise.allpattern (1.2.78+)The comments documenting each pattern variant are helpful.
281-294: LGTM! Pattern-driven symbol matching with good error handling.Using
FindSymbolWithPatternto capture both the symbols and the matching pattern enables precise subsequent replacements. The error messaging appropriately guides users when patterns don't match.
335-347: LGTM! Correct use of matched patterns for replacements.Using
matchedReactPatternandmatchedElementPatterninReplaceOnceensures the replacement targets exactly the pattern that was matched during symbol extraction, preventing mismatches between detection and replacement phases.
- Remove defer attribute accidentally left from testing - Revert Image Snackbar regex change left from testing - Fix regex typo: \[ -> [ in createElement pattern
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/apply/apply.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/apply/apply.go (1)
src/utils/utils.go (2)
FindSymbolWithPattern(318-332)ReplaceOnce(175-188)
🔇 Additional comments (4)
src/apply/apply.go (4)
273-279: Past review concern appears resolved.The past review comment mentioned a potential regex typo with
\[at line 278, but the current code correctly uses[\w_\$](a character class) rather than\[\w_\$](escaped bracket). The pattern should now properly match identifiers likeX.createElement.
281-288: LGTM - pattern-driven approach improves flexibility.The use of
FindSymbolWithPatternwith pattern arrays provides better flexibility for handling different Spotify versions compared to hard-coded patterns. The function correctly returns both the matched symbols and the pattern that matched, enabling dynamic replacement logic.
337-344: LGTM - dynamic pattern usage ensures precise replacements.Using the matched patterns from
FindSymbolWithPatternfor theReplaceOncecalls ensures that replacements occur at the exact locations that were successfully matched. This approach is more robust than using fixed patterns.
263-271: Verify the async pattern completeness (line 268) by testing against actual code.The async pattern on line 268 does not explicitly match the closing
}before the final). According to the comment, the expected structure ism.lazy(async()=>{...await o.e(123).then(...)}), but the regex ends at\.then\(\w+\.bind\(\w+,\d+\)\)without capturing the closing braces. Verify whether this partial pattern is intentional for symbol extraction or if it should be\.then\(\w+\.bind\(\w+,\d+\)\)\}\)to match the complete structure.
| REACT_ELEMENT_REGEX}) | ||
| elementPatterns) | ||
|
|
||
| if (len(reactSymbs) < 2) || (len(eleSymbs) == 0) { |
There was a problem hiding this comment.
Insufficient bounds checking could cause index out of bounds.
The validation checks len(reactSymbs) < 2 and len(eleSymbs) == 0, but the code accesses reactSymbs[2] (line 318) and eleSymbs[2] (lines 305-308, 323). Since all patterns capture exactly 3 groups, the validation should ensure both arrays have at least 3 elements.
Apply this diff to fix the bounds check:
- if (len(reactSymbs) < 2) || (len(eleSymbs) == 0) {
+ if (len(reactSymbs) < 3) || (len(eleSymbs) < 3) {🤖 Prompt for AI Agents
In src/apply/apply.go around line 290, the current validation uses
(len(reactSymbs) < 2) || (len(eleSymbs) == 0) which is insufficient because the
code later accesses reactSymbs[2] and eleSymbs[2]; update the condition to
require at least 3 elements in both slices (e.g., if len(reactSymbs) < 3 ||
len(eleSymbs) < 3) and adjust the error handling path accordingly so you never
index out of bounds.
|
Removed, this was leftover from testing, sorry about that |
|
and what about another coderabbit's review? |
rxri
changed the title
1.2.78+
|
i will fix snackbar thingies with my local commit |
|
@pyyupsk did custom apps work properly on your end? aka could you access them as fine? because your patch works, just they don't display anything :) |
custom apps load fine, but marketplace shows blank (no extensions/themes/snippets). issue is in the marketplace repo it accesses |
|
also some files use the old marketplace url |
Ohhh. Weird that it does not throw an error tho. Okay. Will just do it to wait for it I guess? |
|
I mean, lyrics-plus from what i see is also broken so dunno if we should do something on our end or just let devs update their custom apps tbh |
|
dont know bro, if i have a time ill try to fix it again |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
4 participants
Summary
Test plan
Relates to #3601
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.