Skip to content

Dashboard: Allow editing Local DNS records inline - #3794

Open
brazier wants to merge 8 commits into
pi-hole:developmentfrom
brazier:feat/dnsrecords-edit
Open

Dashboard: Allow editing Local DNS records inline#3794
brazier wants to merge 8 commits into
pi-hole:developmentfrom
brazier:feat/dnsrecords-edit

Conversation

@brazier

@brazier brazier commented Jun 3, 2026

Copy link
Copy Markdown

What does this PR aim to accomplish?:
Allows editing DNS records instead of manually deleting and re-adding

Skjermbilde 2026-06-03 225827

How does this PR accomplish the above?:
Added an edit button that that inserts Inline fileds with the content already inserted, alowing it to be changed, then saved.

Notes:

  • Im not to happy with the width of the icons as they differ, fa-fw makes them to wide(1.25 em) would preferd to set them to 1 em but wanted input here.

  • As i was working with this i did notice that some tend to use i while others span for fa icons so i didnt think so much about which to use for consistency. Can rewrite if there is a new preferred one.


By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code and I have tested my changes.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)
  6. I have checked that another pull request for this purpose does not exist.
  7. I have considered, and confirmed that this submission will be valuable to others.
  8. I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  9. I give this submission freely, and claim no ownership to its content.

  • I have read the above and my PR is ready for review. Check this box to confirm

brazier added 4 commits June 3, 2026 21:17
Fix typo

Signed-off-by: Alexander H. Olsen <alexander@linux.com>
* Changed the layout to a one column full with stacking one.
* Added the option to edit dns/cname records

Signed-off-by: Alexander H. Olsen <alexander@linux.com>
Refactor poc to use the same button helper.

Signed-off-by: Alexander H. Olsen <alexander@linux.com>
Merge upstream development

Signed-off-by: Alexander H. Olsen <alexander@linux.com>
@brazier
brazier requested a review from a team as a code owner June 3, 2026 20:14
@yubiuser

yubiuser commented Jun 3, 2026

Copy link
Copy Markdown
Member

I went with v as both the master and dev branched used that in settings-dns-records.js prior.

This is reverted in dev with #3778

@rdwebdesign

Copy link
Copy Markdown
Member

I have a lot of Local DNS records and always felt the the 2 column layout was small and didnt match the rest of Pi-hole.

Note:

This only happens when using the "boxed layout".

Your new layout will be too wide on big screens.

brazier added 2 commits June 3, 2026 22:39
Prettier fail CI

Signed-off-by: Alexander H. Olsen <alexander@linux.com>
Revert back to a 2 column layout, works better with non-boxed mode.

Signed-off-by: Alexander H. Olsen <alexander@linux.com>
@brazier

brazier commented Jun 3, 2026

Copy link
Copy Markdown
Author

I have a lot of Local DNS records and always felt the the 2 column layout was small and didnt match the rest of Pi-hole.

Note:

This only happens when using the "boxed layout".

Your new layout will be too wide on big screens.

I have never noticed that before after using pi-hole for so many years. Thanks! and reverted. And now rocking the non-boxed mode.

@brazier

brazier commented Jun 3, 2026

Copy link
Copy Markdown
Author

Ill fix the failures first.

XO fixes

Signed-off-by: Alexander H. Olsen <alexander@linux.com>
@rdwebdesign

Copy link
Copy Markdown
Member

Just one more question:

I also tend to change records often for testing or other purposes. Delteting and then re-adding felt uneccesary.

Did you try to use the Advanced settings page to edit this?

If you need to change the list too often, using the advanced settings would be a lot easier:

image image

@brazier

brazier commented Jun 3, 2026

Copy link
Copy Markdown
Author

Just one more question:

I also tend to change records often for testing or other purposes. Delteting and then re-adding felt uneccesary.

Did you try to use the Advanced settings page to edit this?

If you need to change the list too often, using the advanced settings would be a lot easier:
....

yes, but it wasnt what i wanted. there is no way to sort that list, subdomains that are added later end up in the incorrect spots. If i were to edit them in advanced i may aswell edit the file directly on the filesystem instead.

Signed-off-by: Alexander H. Olsen <alexander@linux.com>
@DL6ER DL6ER changed the title Dashboard: Local DNS Records, 1column layout + option of editing records. Dashboard: Allow editing Local DNS records inline Jul 12, 2026

@DL6ER DL6ER left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for reverting the layout - the inline edit feature is the valuable part here. A few things before this can go in:

1. The edit is a non-atomic DELETE-then-PUT and can lose data. updateHostsRecord and updateCnameRecord both DELETE the old record first and only then PUT the new one:

$.ajax({ url: deleteUrl, method: "DELETE" })
  .then(() => $.ajax({ url: putUrl, method: "PUT" }))

If the DELETE succeeds but the PUT fails (e.g., the API rejects the new value), the original record is gone for good, while the .fail branch shows Error while updating ... <oldTag>, which reads as if nothing changed. Since there is no atomic rename endpoint, please reverse the order: PUT the new record first, then DELETE the old one. The worst case then is a harmless duplicate the user can remove, instead of silent data loss.

2. Clearing the TTL silently drops it. In buildCnameRecord, an empty or non-numeric TTL simply omits the segment, so editing a record that had a TTL and blanking the field reverts it to the default with no indication. Please confirm this is intended, or keep the existing TTL when the field is left empty.

3. Description is now stale. I have updated the PR title (the "1column layout" part was reverted). Could you also trim the struck-through layout rationale from the description so it matches what the PR actually does now.

Minor, non-blocking: type="numeric" on the TTL edit input is not a valid input type (it falls back to text), it matches the existing #Cttl field. Both should be fixed together to number

For the record, the env-var handling is a nice catch: the old const setByEnv = false meant the row buttons were never disabled for env-set records, and this fixes that.

@brazier

brazier commented Jul 12, 2026

Copy link
Copy Markdown
Author

@DL6ER Thanks for the input.

  1. I have been using this for a while without any issues, but a sample size of one is not good enough as to say it never will happen. I also dont seem any issues with inverting that.

  2. This is definitly not intended, and just something i just havent tested as honestly i dont use the CNAME that much, i will look into that.

  3. Description is done.

I will also go over the minors once i have a time to sit down. I will need to clone it again and to the changes as i have done further changes, including a better sort, that sorts asc/desc on tld first -> the sld -> subdomain, and the parents are infront of the children. instead of a pure alphabeticly sort, but that is better saved for a second PR.

@rdwebdesign
rdwebdesign requested a review from DL6ER July 14, 2026 17:23
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants