fix: enhance background URL processing to support Windows file paths #1604
Conversation
|
|
WalkthroughThe pull request introduces modifications to the 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/app/app-bg.tsx (1)
57-57: Consider adding an additional check for UNC paths or other edge cases.
While matching/^[a-zA-Z]:\\/.test(originalUrl)correctly identifies drive-letter-based paths, some Windows paths use UNC syntax (e.g., “\server\share”). If such cases need to be supported, consider updating this regex. Otherwise, it might be useful to document the reason for excluding UNC paths.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/app/app-bg.tsx(2 hunks)
🔇 Additional comments (1)
frontend/app/app-bg.tsx (1)
29-29: Confirm we are not over-escaping backslashes.
Replacing single backslashes with double backslashes ensures the CSS parser handles Windows paths correctly. However, if upstream code might already escape these backslashes, this replacement could lead to unintended quadruple backslashes. Please verify (via tests or by auditing upstream path handling) that we do not unintentionally produce over-escaped paths.
✅ Verification successful
Let me gather information about how the cssText is handled before reaching this point.
Let me search for how cssText is processed before reaching this line to understand if there's any prior backslash escaping.
Let me check how the bgAttr value is set and if there's any prior processing of backslashes before it reaches processBackgroundUrls.
Let me check how the CSS parsing is handled and if there are any tests related to backslash handling.
Let me check the complete implementation of the background URL processing and where the CSS text originates from.
Based on the code analysis, I can now provide a definitive response about the backslash escaping:
The backslash escaping is correctly implemented and not causing over-escaping.
The code shows that:
- The
bgAttrvalue comes directly from the tab's metadata without any prior escaping - The
processBackgroundUrlsfunction is the only place where backslash escaping occurs - The CSS parser (
parseCSS) requires backslashes to be escaped to handle Windows paths correctly - The escaping is done only once with
cssText.replace(/\\/g, '\\\\')before parsing - The CSS parser then correctly interprets these escaped backslashes when processing URLs
The escaping is necessary because:
- Windows paths contain single backslashes (e.g.,
C:\path\to\file.jpg) - CSS requires backslashes in URLs to be escaped
- The CSS parser needs to see
\\to interpret it as a single\in the final URL
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for assignments or declarations of cssText
rg "cssText" -B 3 -A 3
# Look for any path handling or backslash escaping logic
rg "\\\\|replace.*\\\\" -B 3 -A 3
Length of output: 67064
Script:
#!/bin/bash
# Look for the function definition and usage of processBackgroundUrls
ast-grep --pattern 'function processBackgroundUrls($$$) {
$$$
}'
# Look for where cssText is assigned/modified before reaching processBackgroundUrls
rg -B 5 -A 5 "processBackgroundUrls"
Length of output: 1550
Script:
#!/bin/bash
# Look for where bg meta is set/modified
rg "meta.*bg.*=" -B 5 -A 5
# Look for any backslash processing in the codebase related to background or meta
rg -B 3 -A 3 "bg.*replace.*\\\\"
rg -B 3 -A 3 "meta.*replace.*\\\\"
Length of output: 1762
Script:
#!/bin/bash
# Look for CSS parsing tests and backslash handling
rg -B 3 -A 3 "parseCSS.*test"
# Check the CSS parser implementation
ast-grep --pattern 'parseCSS($$$) {
$$$
}'
# Look for any test cases involving backslashes in background URLs
rg -B 3 -A 3 "background.*\\\\.*test"
Length of output: 116
Script:
#!/bin/bash
# Get the full implementation of app-bg.tsx
cat frontend/app/app-bg.tsx
# Look for where background URLs are set in the codebase
rg -B 5 -A 5 "url\(['\"]?[^'\")]+['\"]?\)"
Length of output: 69011
|
@esimkowitz the testdriver ai onboarding failed. does it require any actions from my side? |
|
@ianjennings @chottuthejimmy could one of you take a look? Doesn’t seem like the job is getting queued |
Sure give a moment |
|
@esimkowitz yes it wasn't triggered, I see nothing at our backend. |
|
It is working fine on the main branch |
|
@chottuthejimmy okay queued to rerun. Yeah I've noticed that the command to queue the job to your service fails intermittently. It seems most of the recent failures have been because of this. |
|
Yeah, I mean I am trying to figure it out. That said will get something to you before tmr. |
It seems like the GitHub secret key is not being set during the run. According to this post it depends who triggers the run. @SamimAB may not have permissions to use GitHub secrets. Try adding them to the org? We can definitely make our errors more apparent, will make a note of that. |
|
Let's move this discussion to #1621 |
|
hi, really appreciate that you brought this issue to our attention! i ended up just making change to the so now you should be able to do something like this in powershell (note if using bash or a unix shell you'd also need to escape the backslashes in the shell as well): the problem with blanket replacing \ => \ everywhere in the CSS is it could have unintended consequences. it would also affect urls that were already properly encoded with backslashes. |
|
@sawka makes sense. appreciate it. will check it out. |
In processBackgroundUrls, made two changes -
/^[a-zA-Z]:\\/.test(originalUrl)I tried a few other fixes but this one works the best with least changes to the codebase. Hopefully this is helpful.