Fix: Add timezone offset to Immich memories "for" query parameter#660
Merged
Conversation
📝 WalkthroughWalkthroughAdds a ChangesTimezone Offset Restoration
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant MemoryAssetsPool
participant ImmichApi
participant ImmichServer
MemoryAssetsPool->>ImmichApi: SearchMemoriesAsync(for)
ImmichApi->>ImmichApi: PrepareRequest(url)
ImmichApi->>ImmichApi: EnsureTimezoneOffset(url)
ImmichApi->>ImmichServer: GET /memories?for=...zzz
ImmichServer-->>ImmichApi: 200 OK
ImmichApi-->>MemoryAssetsPool: memories result
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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)
ImmichFrame.Core/Api/ImmichApi.cs (1)
58-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffManual query-string index math is fragile; consider
HttpUtility.ParseQueryString.Locating and replacing the parameter value via raw
IndexOf/Substringon the URL string works but is easy to get subtly wrong on future edits (e.g., if NSwag ever double-encodes or reorders parameters). UsingSystem.Web.HttpUtility.ParseQueryString(or splitting on?/&more defensively) would reduce that risk.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ImmichFrame.Core/Api/ImmichApi.cs` around lines 58 - 70, The query-parameter extraction logic in ImmichApi is using fragile manual IndexOf/Substring parsing, which can break with encoding or parameter ordering changes. Update the URL handling in the relevant parameter-replacement helper to parse the query string more defensively, preferably via HttpUtility.ParseQueryString, and use the parsed collection to locate and replace the target parameter value instead of relying on raw string index math.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ImmichFrame.Core/Api/ImmichApi.cs`:
- Around line 56-87: Add direct unit tests for EnsureTimezoneOffset in ImmichApi
to cover its branching behavior: missing parameter, missing query delimiter,
unparsable value, and unchanged value when the offset is already correct. Use
the method name EnsureTimezoneOffset as the target and assert the resulting
query string after rewriting so the tests validate the exact URL mutation rather
than only downstream LoadAssets behavior.
---
Nitpick comments:
In `@ImmichFrame.Core/Api/ImmichApi.cs`:
- Around line 58-70: The query-parameter extraction logic in ImmichApi is using
fragile manual IndexOf/Substring parsing, which can break with encoding or
parameter ordering changes. Update the URL handling in the relevant
parameter-replacement helper to parse the query string more defensively,
preferably via HttpUtility.ParseQueryString, and use the parsed collection to
locate and replace the target parameter value instead of relying on raw string
index math.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bf358a05-9553-434e-9e96-c771b017d55a
📒 Files selected for processing (2)
ImmichFrame.Core/Api/ImmichApi.csImmichFrame.Core/Logic/Pool/MemoryAssetsPool.cs
Comment on lines
+56
to
+87
| private static void EnsureTimezoneOffset(System.Text.StringBuilder urlBuilder, string parameterName) | ||
| { | ||
| var url = urlBuilder.ToString(); | ||
| var token = parameterName + "="; | ||
|
|
||
| var idx = url.IndexOf("?" + token, StringComparison.Ordinal); | ||
| if (idx < 0) idx = url.IndexOf("&" + token, StringComparison.Ordinal); | ||
| if (idx < 0) return; | ||
|
|
||
| var valueStart = idx + 1 + token.Length; | ||
| var valueEnd = url.IndexOf('&', valueStart); | ||
| if (valueEnd < 0) valueEnd = url.Length; | ||
|
|
||
| var encodedValue = url.Substring(valueStart, valueEnd - valueStart); | ||
| var rawValue = Uri.UnescapeDataString(encodedValue); | ||
|
|
||
| // Only rewrite values that look like a datetime; leave anything else untouched. | ||
| // The original offset is already lost at this point (stripped by "s"), so we re-infer it | ||
| // with AssumeLocal, i.e. the machine's local offset. This is correct for the only caller | ||
| // today (SearchMemoriesAsync with DateTimeOffset.Now). If a caller ever passes a value | ||
| // with a different offset (e.g. UTC), it would be normalized to the local offset here. | ||
| if (!DateTimeOffset.TryParse(rawValue, System.Globalization.CultureInfo.InvariantCulture, | ||
| System.Globalization.DateTimeStyles.AssumeLocal, out var value)) | ||
| return; | ||
|
|
||
| var fixedValue = Uri.EscapeDataString( | ||
| value.ToString("yyyy-MM-ddTHH:mm:sszzz", System.Globalization.CultureInfo.InvariantCulture)); | ||
| if (fixedValue == encodedValue) return; | ||
|
|
||
| urlBuilder.Remove(valueStart, valueEnd - valueStart); | ||
| urlBuilder.Insert(valueStart, fixedValue); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add unit tests for EnsureTimezoneOffset.
This method fixes a production-breaking 400 error and contains several branches (missing parameter, missing delimiter, unparsable value, unchanged value). Direct unit tests covering these cases (e.g., asserting the rewritten query string) would give higher confidence than relying solely on downstream LoadAssets tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ImmichFrame.Core/Api/ImmichApi.cs` around lines 56 - 87, Add direct unit
tests for EnsureTimezoneOffset in ImmichApi to cover its branching behavior:
missing parameter, missing query delimiter, unparsable value, and unchanged
value when the offset is already correct. Use the method name
EnsureTimezoneOffset as the target and assert the resulting query string after
rewriting so the tests validate the exact URL mutation rather than only
downstream LoadAssets behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #659
Summary by CodeRabbit