Skip to content

fix(dashboard): preserve issue row urls#96

Open
corylanou wants to merge 1 commit into
mainfrom
symphony/digitaldrywood_symphony_93
Open

fix(dashboard): preserve issue row urls#96
corylanou wants to merge 1 commit into
mainfrom
symphony/digitaldrywood_symphony_93

Conversation

@corylanou
Copy link
Copy Markdown

Context

Fixes #93. Dashboard issue URL controls need stable row URLs after recent entry-shape changes.

TL;DR

Keep dashboard issue URL buttons wired across running, retry, and recent rows.

Summary

  • Normalize orchestrator issue fields through one helper for runtime and retry entries.
  • Preserve URL, title, and description when refreshing, blocking, retrying, and snapshotting.
  • Add presenter and LiveView regressions for running, retry, and recent URL controls.

Alternatives

  • Only patching presenter fallbacks would leave duplicated orchestrator entry shapes fragile.

Test Plan

  • make -C elixir all
  • mix test test/symphony_elixir/orchestrator_status_test.exs test/symphony_elixir_web/dashboard_live_test.exs
  • mix test test/symphony_elixir/extensions_test.exs

Summary:
- Normalize orchestrator issue entry fields for running, retry, blocked,
  and snapshot payloads.
- Add regression coverage for presenter issue URLs and dashboard URL
  controls across running, retry, and recent rows.

Rationale:
- The dashboard controls depend on a stable issue URL field, but that
  shape was rebuilt in several places and could silently drift.

Tests:
- mix test test/symphony_elixir/orchestrator_status_test.exs test/symphony_elixir_web/dashboard_live_test.exs
- mix test test/symphony_elixir/extensions_test.exs
- make all

Co-authored-by: Codex <codex@openai.com>
@corylanou
Copy link
Copy Markdown
Author

@codex review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f398a3621a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +897 to +899
running_entry
|> Map.merge(issue_entry_fields(issue))
|> Map.put(:issue, issue)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid clearing cached issue fields on running refresh

refresh_running_issue_state/2 now does Map.merge(issue_entry_fields(issue)), which overwrites previously cached :url, :title, and :description with nil when a refreshed %Issue{} is partial. In that case the dashboard loses issue URL controls for still-running rows (and the same pattern is used for blocked rows), whereas the previous behavior preserved the cached top-level fields as a fallback. This shows up when a tracker intermittently omits those fields during poll refreshes.

Useful? React with 👍 / 👎.

@corylanou
Copy link
Copy Markdown
Author

Rework

P2 from Codex on f398a36 — substantively important (re-introduces the original bug):

refresh_running_issue_state/2 now does Map.merge(issue_entry_fields(issue)), which overwrites previously cached :url, :title, and :description with potentially-nil values from the new fetch.

This means the icons LOSE their URL on every refresh cycle — exactly the bug this PR is supposed to lock down. The icons will disappear again after the next state refresh.

Fix: Don't Map.merge with the new fetch values. Instead, only overwrite a cached field if the new value is non-nil. A helper like:

defp merge_keeping_non_nil(existing, new) do
  Enum.reduce(new, existing, fn
    {_k, nil}, acc -> acc
    {k, v}, acc -> Map.put(acc, k, v)
  end)
end

Critical for the regression test: add a test asserting that after a refresh cycle where the fetch returns nil for url, the cached URL is preserved (not nilled). That's the actual lock-in. Without that test, this same bug returns next refactor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

regression(dashboard): copy + open-in-new-tab icons disappear on most rows after #82

1 participant