rebase

[leonarduk]

No description provided.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[github-actions]

GPT AI Code Review

1. Acceptance criteria

There are no specific acceptance criteria provided in a linked issue, and the PR title "rebase" does not indicate any particular feature or bug fix. The changes made in the diff primarily involve the removal of service worker-related code and some updates to dependencies. Since there are no clear acceptance criteria to evaluate against, it is difficult to assess if the PR meets any specific requirements.

2. Bugs and logic errors

The removal of service worker functionality could lead to issues if push notifications were previously expected to work. The code related to push notifications has been entirely removed from the AlertSettings, Support, and other components, which could break existing functionality if users were relying on these features. If the intention is to remove push notifications entirely, this should be clearly documented, and users should be informed of the change.

3. API, data, and workflow safety

• The removal of service worker code and related push notification logic could lead to inconsistencies in the application if the backend still expects to handle push notifications. This needs to be verified to ensure that the backend and frontend contracts are still aligned.
• The changes do not appear to break local smoke tests or deployment workflows directly, but the removal of service worker functionality could impact how the app behaves in production, especially regarding offline capabilities and notifications.
• There are no apparent issues with secrets, permissions, or CI assumptions in the changes made.

4. Test coverage

There is no indication that tests have been updated or added to cover the removal of service worker functionality or the changes made to the components. Given that push notifications are a significant feature, it would be prudent to ensure that tests are in place to validate the expected behavior of the application without this functionality.

5. Minor issues (optional)

• The removal of the service worker and related code should be documented in the README or release notes to inform users of the changes.
• The changes in the GroupPortfolioView and AllocationCharts components to handle optional parameters (e.g., n?: string) are a good practice, but it would be beneficial to ensure that this is consistently applied across the codebase.

Summary Verdict: REQUEST CHANGES - The removal of service worker functionality needs to be clarified, and tests should be updated or added to ensure that the application behaves as expected without this feature. Additionally, documentation should be updated to reflect these changes.

---

Reviewed by GPT via gpt-pr-review.yml. Advisory only.

[github-actions]

Claude AI Code Review

PR Review: rebase

1. Acceptance criteria

No linked issue. Reviewing on merits: the PR appears to be cleaning up import cycles, hardening FX validation, improving meta-lookup fallback chains, and aligning test monkeypatching targets. All stated goals are reflected in the diff.

---

2. Bugs and logic errors

Significant: test_load_latest_prices_skips_when_currency_unknown asserts wrong behaviour

When get_instrument_meta returns {}, the code path is:

raw_currency = str(meta.get("currency") or "GBP").strip()  # → "GBP"
currency = raw_currency.upper()  # → "GBP"

The elif currency != "GBP" branch is skipped entirely, so val passes through unchanged and result[key] = val is set. The test asserts prices == {} but the actual return should be {"ABC.L": 10.0}. This test will fail (or passes currently only because something else causes a skip that isn't guaranteed). Verify before merge.

Significant: test_load_latest_prices_resolution_scaling_and_missing — ABC result changed without justification

The existing test expected ABC.L: 20.0 (from Close_gbp), but the new assertion is ABC.L: 10.0. The Close_gbp column is present in the fake data and the code still has if close_gbp_col is None: guarding FX conversion — so Close_gbp = 20.0 should still be preferred. The get_scaling_override for ABC returns 0.5, and apply_scaling is not monkeypatched, so if it multiplies the Close_gbp column by 0.5 that would explain 10.0. But then apply_scaling is also being applied to Close_gbp values, which contradicts the contract ("If Close_gbp exists, use it directly"). This needs a comment or the scaling logic needs to be verified not to mutate Close_gbp.

Minor: Double pd.notna check on fx_rate

if not fx_rate or not pd.notna(fx_rate) or not pd.api.types.is_number(fx_rate):
    continue
fx_rate = float(fx_rate)
if not pd.notna(fx_rate) or fx_rate <= 0:   # pd.notna(float) is always True
    continue

The second pd.notna after float() conversion is always True (a Python float is never pd.NA). Not harmful but noisy. The not fx_rate check will also skip a valid fx_rate = 0.0, which is correct, but it will also skip anything falsy in a way that duplicates the <= 0 check. Simplify to:

if not isinstance(fx_rate, (int, float)) or not math.isfinite(fx_rate) or fx_rate <= 0:
    continue

Minor: pd.api.types.is_number doesn't exist

The correct API is pd.api.types.is_number(fx_rate) — this function does exist in pandas, but it checks dtype of a scalar, not whether a Python object is numeric. For a Python float, it returns True, so it won't cause failures here, but it's semantically wrong. Use isinstance(fx_rate, (int, float)) or np.isfinite.

Minor: raise_for_status refactor in load_live_prices

raise_for_status = getattr(resp, "raise_for_status", None)
if callable(raise_for_status):
    raise_for_status()

requests.Response always has raise_for_status. This looks like it was done to accommodate mock objects in tests, but it silently swallows HTTP errors if the attribute is missing. The original resp.raise_for_status() was safer. If the goal is testability, mock the response properly or use unittest.mock.

---

3. API, data, and workflow safety

Import cycle fix is correct in approach — the lazy from backend.common import portfolio_utils inside _fx_to_base wrapper is standard Python cycle-breaking. No issues.

Meta lookup fallback chain (get_instrument_meta(full_ticker) or get_instrument_meta(full) or get_instrument_meta(ticker)) is an improvement for resilience. Low risk.

ts is None change — changing not ts to ts is None means a timestamp of 0 (Unix epoch) is now accepted. This is technically correct for the Yahoo API but practically harmless.

No frontend contract changes — payload shapes appear unchanged.

No CI/CDK/deployment files touched. Safe.

---

4. Test coverage

Good additions:

• test_load_latest_prices_skips_invalid_fx_rate — covers 0.0, None, nan — valuable.
• FX monkeypatch target corrected to holding_utils._fx_to_base throughout — this is the right fix; previously tests were patching portfolio_utils._fx_to_base which would not intercept the wrapper.

Missing cases:

• No test verifying that apply_scaling does not corrupt the Close_gbp column (relevant to the ABC result discrepancy above).
• No test for load_live_prices with an invalid/zero FX rate (the new guard code paths in load_live_prices are untested).
• load_live_prices with ts = 0 (the ts is None change).

---

5. Minor issues

• Removing the # backend/common/holding_utils.py file header comment is fine.
• The trailing newline fix in _cash_name is correct.
• Blank line added before if pass_price_hint: — fine.
• The docstring rewrite for load_latest_prices is clearer. Good.

---

Verdict

REQUEST CHANGES

---

Reviewed by Claude via claude-pr-review.yml. Advisory only.