Skip to content

v0.15.1#170

Merged
yimsk merged 2 commits intomainfrom
develop
Feb 24, 2026
Merged

v0.15.1#170
yimsk merged 2 commits intomainfrom
develop

Conversation

@yimsk
Copy link
Copy Markdown
Contributor

@yimsk yimsk commented Feb 24, 2026

Summary

@claude
Copy link
Copy Markdown

claude Bot commented Feb 24, 2026

Code Review

Overall this is a clean, useful addition. A few things worth addressing:


Bug: svc.Deployments() called twice per loop iteration

render.go:80-81:

for i := range svc.Deployments() {
    dep := &svc.Deployments()[i]

Deployments() is called on every iteration to get both the range and the element. While the method is a simple return r.Item.Deployments so it won't diverge, taking &svc.Deployments()[i] is taking an address into a different (though identically-backed) slice header than the range expression evaluated. Store the result once:

deployments := svc.Deployments()
for i := range deployments {
    dep := &deployments[i]

Logic issue: mixing UpdatedAt and CreatedAt in the same comparison

render.go:83-94:

if dep.UpdatedAt != nil && (latestTime == nil || dep.UpdatedAt.After(*latestTime)) {
    latest = dep
    latestTime = dep.UpdatedAt
    continue
}
if dep.CreatedAt != nil && (latestTime == nil || dep.CreatedAt.After(*latestTime)) {
    latest = dep
    latestTime = dep.CreatedAt
}

Consider a case where deployment A sets latestTime = A.UpdatedAt = T1, and then deployment B has B.UpdatedAt = T0 < T1 (first if fails, no continue) but B.CreatedAt = T2 > T1 (second if succeeds). The result is that B is selected as "latest" based on CreatedAt, even though A has a more recent UpdatedAt. This seems unlikely in practice but is logically inconsistent.

A cleaner approach picks the best available timestamp per deployment before comparing:

deployments := svc.Deployments()
for i := range deployments {
    dep := &deployments[i]
    var t *time.Time
    switch {
    case dep.UpdatedAt != nil:
        t = dep.UpdatedAt
    case dep.CreatedAt != nil:
        t = dep.CreatedAt
    }
    if t != nil && (latestTime == nil || t.After(*latestTime)) {
        latest = dep
        latestTime = t
    }
}

Minor: inconsistent fallback value on type assertion failure

Other column getters (getStatus, getDesired, etc.) return "" when the type assertion to *ServiceResource fails. getLastDeployment returns "-". Either is fine but it's worth being consistent.


Missing tests

There are no tests in the services package, and none are added here. The logic in getLatestDeployment (especially the timestamp selection) would benefit from a unit test covering edge cases (nil UpdatedAt, nil CreatedAt, empty deployments slice, multiple deployments). Low priority given the rest of the codebase appears to be in the same state, but the logic bug above makes tests more worthwhile.


Nit: column width

Width 26 for "LAST DEPLOYMENT" fits the header (15 chars) comfortably, and "in progress - 365d" is 18 chars. Looks fine.


Summary: The feature is useful and the code is readable. The logic bug in getLatestDeployment (mixing UpdatedAt/CreatedAt in the cross-deployment comparison) is the main thing worth fixing before merging.

@yimsk yimsk merged commit 87d9644 into main Feb 24, 2026
12 checks passed
tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request Feb 24, 2026
This MR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [github:clawscli/claws](https://github.com/clawscli/claws) | patch | `0.15.0` → `0.15.1` |

MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot).

**Proposed changes to behavior should be submitted there as MRs.**

---

### Release Notes

<details>
<summary>clawscli/claws (github:clawscli/claws)</summary>

### [`v0.15.1`](https://github.com/clawscli/claws/releases/tag/v0.15.1)

[Compare Source](clawscli/claws@v0.15.0...v0.15.1)

#### Changelog

- [`4fe75b6`](clawscli/claws@4fe75b6) Add docker support and github CR package workflow
- [`92189e5`](clawscli/claws@92189e5) Cross-compile for multiple archs instead of relying on QEMU
- [`63731ed`](clawscli/claws@63731ed) Merge pull request [#&#8203;165](clawscli/claws#165) from clawscli/develop
- [`d806a8e`](clawscli/claws@d806a8e) Merge pull request [#&#8203;167](clawscli/claws#167) from stefan-matic/main
- [`f508583`](clawscli/claws@f508583) Merge pull request [#&#8203;168](clawscli/claws#168) from clawscli/develop
- [`7fbf05d`](clawscli/claws@7fbf05d) Merge pull request [#&#8203;169](clawscli/claws#169) from dpalvolgyi-pw/feature/ecs-last-deployment
- [`87d9644`](clawscli/claws@87d9644) Merge pull request [#&#8203;170](clawscli/claws#170) from clawscli/develop
- [`6d22155`](clawscli/claws@6d22155) feat(ecs): add last deployment column

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this MR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My4zMS4xIiwidXBkYXRlZEluVmVyIjoiNDMuMzEuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiUmVub3ZhdGUgQm90IiwiYXV0b21hdGlvbjpib3QtYXV0aG9yZWQiLCJkZXBlbmRlbmN5LXR5cGU6OnBhdGNoIl19-->
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.

2 participants